-
-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve directed infinity #809
Conversation
@@ -102,15 +100,10 @@ def quiet_f(*args): | |||
|
|||
return quiet_f | |||
expr: Optional[Type[BaseElement]] = Expression(SymbolN, expr).evaluate(evaluation) | |||
quiet_expr = Expression( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came up when I run the doctests with the other changes: In WMA, when the points are evaluated, *Plot
hides all the warnings, not just "Power::infy". This could go in a different PR, but the natural tests needs of the changes introduced here. (I also tested with functions like F[x_]:=(Open["nonexistingfile"];x^2)
but it is just not so nice.
CHANGES.rst
Outdated
@@ -3,6 +3,25 @@ | |||
CHANGES | |||
======= | |||
|
|||
|
|||
API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually API refers the external API - things that a person using core would have to worry about.
I suspect this is just about internal organization and under the general category of we are moving evaluation functions into its own module, separate from the eval methods that function application finds.
mathics/builtin/arithmetic.py
Outdated
} | ||
|
||
formats = { | ||
"DirectedInfinity[1]": "HoldForm[Infinity]", | ||
"DirectedInfinity[-1]": "HoldForm[-Infinity]", | ||
"DirectedInfinity[-1]": "PrecedenceForm[-HoldForm[Infinity], 390]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be getting numbers like 390 from the operator tables of Mathics scanner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Times has Precedence 400, this needs to have a slightly lower value to ensure the parenthesis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is something then incomplete here. What comes in contact with what? Is there a value in Robert's table that is correct here?
And finally, if there is something that needs to be adjusted from the normal value of some other operator, a clearer way to do that would be to use that value and add or subtract from it. Something like Precedence["Times"] - xxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe what is missing is better documentation of what PrecedenceForm
means.
https://reference.wolfram.com/language/ref/PrecedenceForm.html
In WMA, the right value for the precedence would be exactly the same as Times (400):
In[1]:= Times[b, PrecedenceForm[3 A ,400]]
Out[1]= b (3 A)
In[2]:= Times[b, PrecedenceForm[3 A ,401]]
Out[2]= b 3 A
In Mathics, this does not works in the same way:
In[1]:= Times[b, PrecedenceForm[3 A ,400]]
Out[1]= b (3 A)
In[2]:= Times[b, PrecedenceForm[3 A ,401]]
Out[2]= b (3 A)
In[2]:= Times[b, PrecedenceForm[3 A ,399]]
Out[2]= b (3 A)
but
In[1]:= Times[b, PrecedenceForm[A ,400]]
Out[1]= b A
In[2]:= Times[b, PrecedenceForm[3 A ,401]]
Out[2]= b A
In[2]:= Times[b, PrecedenceForm[3 A ,399]]
Out[2]= b (A)
So maybe we need to adjust this before worrying about the exact value for the precedence in this rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good and important information. Give me some time to study this and compare with the existing tables and Robert's tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice also that Robert's tables have several values depending on the context. I guess that this is related to the use of PrecedenceForm in some internal the format rules in WMA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is a much larger issue, and I think it is not going to be solved in the way proposed above, let's remove this change. I created #811 to capture the discussion above, and track this problem.
mathics/builtin/arithmetic.py
Outdated
"DirectedInfinity[]": "HoldForm[ComplexInfinity]", | ||
"DirectedInfinity[DirectedInfinity[z_]]": "DirectedInfinity[z]", | ||
"DirectedInfinity[z_?NumericQ]": "HoldForm[z Infinity]", | ||
"DirectedInfinity[z_]": "PrecedenceForm[z HoldForm[Infinity], 390]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
390 - see above.
@@ -164,7 +177,7 @@ def test_multiply(str_expr, str_expected, msg): | |||
("I^(2/3)", "(-1) ^ (1 / 3)", None), | |||
# In WMA, the next test would return ``-(-I)^(2/3)`` | |||
# which is less compact and elegant... | |||
("(-I)^(2/3)", "(-1) ^ (-1 / 3)", None), | |||
# ("(-I)^(2/3)", "(-1) ^ (-1 / 3)", None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I have it correct that this was working before, and not it is not? If that it the case, what's the insight here. (And if not, what is the insight here.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works in #766 but not here. The issue comes from Power.eval_check
.
mathics/builtin/arithmetic.py
Outdated
@@ -208,6 +208,13 @@ class Abs(_MPMathFunction): | |||
summary_text = "absolute value of a number" | |||
sympy_name = "Abs" | |||
|
|||
def eval(self, x, evaluation): | |||
"%(name)s[x_]" | |||
result = eval_abs(x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should follow convention using capital letter. Also it would be nice to start removing %(name)s
.
I will be putting in a PR to merge into this to correct such things.
CHANGES.rst ----------- this is a continuation of prior work, not an external API change arithmetic.py ------------- * eval_{abs,sign} -> eval_{Abs,Sign}; * remove MakeBoxes hack - will handle by a more pervasive go-over of Precedence. * generic style things changes. - Split long URL lines - reduce use of %(name)s - order class definitions test_basic.py ------------- Isolate failing DirectedInfinity tests
Improve directed infinity
After CI clears - LGTM |
This is based on one part of #766 related to how DirectedInfinity is handled. To simplify this, I remove part of the
eval_Times
code handling this kind of expression and replace it with upvalues rules. Also, some of the new pytests in #766 were included here.