Arithmetic refactor #766

mmatera · 2023-01-27T22:34:18Z

Well, it took a lot of time, but I think this is now ready for revision. The idea is to move the code associate to the evaluation of arithmetic expressions, especially those expressions composed mainly of numbers and arithmetic operations, to a dedicated module in mathics.core.eval.

There are several motivations to do this. In the first place, the code in master is quite involved, and not very well documented. Also, implies conversions back and forward to SymPy. Moreover, the way in which sympy evals arithmetic expressions is not fully compatible with the expected behavior in WMA. Then, more conversions and hacking over the returned code were needed.
With the code in this branch, I hope the logic is a little bit clearer, and several useless conversions are avoided. Also, the issue with DirectedInfinity that prevents the application of some rules associated with Gudermannian and other complex functions should be fixed now.

mmatera · 2023-01-27T22:38:06Z

mathics/eval/arithmetic.py

+}
+
+
+# Here I used the convention of calling eval_* to functions that can produce a new expression, or None


@rocky, I leave this long comment here about the eval<-> apply semantics. Maybe we want to go over this again before merging this.

I looked at this and there is information here that I a do not believe is elsewhere. I think it should be elsewhere but I don't know right now where that elsewhere is.

mmatera · 2023-01-27T22:49:50Z

mathics/builtin/numbers/hyperbolic.py

        # Handled already
-        # "Gudermannian[Infinity]": "Pi/2",
+        "Gudermannian[DirectedInfinity[1]]": "Pi/2",


Notice that the rule requires to use DirectedInfinity[1] instead of Infinity. This is due to Infinity is evaluated to DirectedInfinity[1] before looking at the Gudermannian set of rules.

Ok - Thanks for the explanation. I'd like to understand this more though. What is it that turns Infinity into DirectedInfinity[1] ? ~~Is this SymPy doing this?~~

As you say below, WMA turns Infinity to DirectedInfinity[1]. I wonder if we should be doing the same thing?

We should if we want to get the same result if we apply the same rules as written for WMA.

Ok. If this is simple for now to do, then let's do. Or we can write DirectedInfinity[1] in the rules and come back to this some other time. Your choice - assuming this is a simple fix.

rocky · 2023-01-28T00:26:35Z

Thanks for looking at and untangling this. With regards to:

Moreover, the way in which sympy evals arithmetic expressions is not fully compatible with the expected behavior in WMA.

I would like to understand this in more detail. Is this documented somewhere? (I am currently looking myself).

What exactly are the differences?

rocky · 2023-01-28T00:29:46Z

With respect to

conversions back and forward to SymPy.

I would like more details here too. Obviously we want to reduce this, but generally the ideal flow is convert into SymPy as much as possible and convert back to Mathics as late or as little as possible. In fact this idea is true for other things as well, like numpy arrays, constant literals that map into Python, etc.

See below for revision to this

rocky · 2023-01-28T00:38:07Z

mathics/eval/arithmetic.py

+        if exp.is_zero:
+            return Integer1
+        if isinstance(exp, (Integer, Real, Rational)):
+            sign = eval_sign(base) or Expression(SymbolSign, base)


My Editor tells and flake8 tell me SymbolSign is not defined.

Thanks for catching this.

I have to add more tests for these routines, but at this point, I need a little bit of feedback (not necessarily right now, but before continuing with this).

rocky · 2023-01-28T00:39:55Z

mathics/eval/arithmetic.py

+        if isinstance(exp, (Integer, Real, Rational)):
+            return Integer1
+        if isinstance(exp, Complex):
+            return Expression(SymbolExp, exp.imag)


My editor and flake8 tell me that SymbolExp is not defined.

With respect to

conversions back and forward to SymPy.

I would like more details here too. Obviously we want to reduce this, but generally the ideal flow is convert into SymPy as much as possible and convert back to Mathics as late or as little as possible. In fact this idea is true for other things as well, like numpy arrays, constant literals that map into Python, etc.

Suppose you are going to evaluate

3^(1/2) + 5 * 3^(-1/2) + 3 * Pi^2

1/2 is converted to SymPy twice back and forward, to see that the expression didn't change, so 1/2 is kept. Then we have the evaluation of 3^(-1/2) and 3^(-1/2) where 3 and 1/2, and 3^(1/2) are converted again back and forward, to again get the same result. And again in the multiplication by 5, and again in the addition, to get at the end the very same expression.

All of this can be addressed by having a special form of Expression that is known to have a SymPy equivalent and that is saved. So 3, 1/2, 3^(1/2) are all Expressions that have SymPy expr object equivalents.

We will probably do the same for literals that have exact Python correspondences, and numerous other things.

If this is the only thing that being addressed, please don't do it this way.

The problem is that for certain expressions, the replacement rules in WMA and SymPy are different, which makes that new rules written in WL fails to be applied, like in the case of DirectedInfinity. Actually, I did all of this because it was not possible to fix these rules without disentangling all the arithmetic module.

I think here we need more knowledgeable conversion routines between the Mathics and SymPy representations.

Guder

That part of the PR are in this branch: https://github.com/Mathics3/mathics-core/tree/tests_from_arithmetic_refactor

However, the new tests do not pass without the other changes.

A number of them do, so those could go in. The ones that don't can be commented out to remind us to address various issues.

@mmatera if you would like me to make a stab and extracting the non-controversial parts, I'll do.

rocky · 2023-01-28T00:42:45Z

mathics/eval/arithmetic.py

+RealOne = Real(1.0)
+
+
+# Here we store numerical constants with their approximate


This smells like a wrong thing to do. I see it is used in test_arithmetic_expr and that is new too, so that is probably not advisable either. What is the history behind these?

In a couple of places in the code, I need to multiply by that constant. The same with RationalMOneHalf. It is just to about avoiding calling the constructor to get the instance.

I didn't mean RealOne, I was referring to SymolE, SymbolEulerGamma, and SymbolPi.

BTW Computing RealOne once and using it many times is fine. And if we find that other modules tend to need RealOne, we can put it in a more global place.

mmatera · 2023-01-28T00:43:39Z

Thanks for looking untangling this. With regards to:

Moreover, the way in which sympy evals arithmetic expressions is not fully compatible with the expected behavior in WMA.

I would like to understand this in more detail. Is this documented somewhere? (I am currently looking myself).

What exactly are the differences?

The first difference that I noticed is that SymPy rationalizes fractions. For example, (1/3)^(1/2) goes to Sqrt[3]/3, but in WMA, the result is 1/Sqrt[3]. Also, there are other conversions, like what happens with Complex numbers, which in SymPy are expressions, not atoms, and how Infinity quantities are handled. In WMA, Infinity and ComplexInfinity are evaluated as DirectedInfinity[1] and DirectedInfinity[].

rocky · 2023-01-28T01:13:51Z

mathics/eval/arithmetic.py

+    return result
+
+
+def distribute_factor(expr: BaseElement, factor: BaseElement) -> BaseElement:


I am not seeing distribute_factor or distribute_powers used anywhere. Where is this used?

rocky · 2023-01-28T01:19:08Z

Thanks for looking untangling this. With regards to:

Moreover, the way in which sympy evals arithmetic expressions is not fully compatible with the expected behavior in WMA.

I would like to understand this in more detail. Is this documented somewhere? (I am currently looking myself).
What exactly are the differences?

The first difference that I noticed is that SymPy rationalizes fractions. For example, (1/3)^(1/2) goes to Sqrt[3]/3, but in WMA, the result is 1/Sqrt[3]. Also, there are other conversions, like what happens with Complex numbers, which in SymPy are expressions, not atoms, and how Infinity quantities are handled. In WMA, Infinity and ComplexInfinity are evaluated as DirectedInfinity[1] and DirectedInfinity[].

This was said above but I think it applies here as well. The real issue is about whether there is loss of information in either representation. If not, then I would first try to get this by doing a better conversion. In a sense this is also related to the Form stuff as well.

It may turn out that this is too tricky, but I strongly encourage starting out with that first (and don't ask me for the reasons why unless you are prepared for the consequences of the answer).

Also, please note that in a Expression that saves the sympy representation, there is no conversion smarts per se. The system has its representation in its nice form and the Sympy expression has its representation in its nice form whether or not we had smarts to know that this is how the two correspond.

rocky · 2023-01-28T01:26:19Z

With respect to

conversions back and forward to SymPy.

I would like more details here too. Obviously we want to reduce this, but generally the ideal flow is convert into SymPy as much as possible and convert back to Mathics as late or as little as possible. In fact this idea is true for other things as well, like numpy arrays, constant literals that map into Python, etc.

So based on what has followed, I have to back off here, but only a little. You know how we have this overly complex caching mechanism for Expressions? Well, we might have something like that for Expressions with sympy representations. However the invalidation/update is done only when a rewrite rule kicks in.

mmatera · 2023-01-28T01:41:16Z

Let me put this in a different way. I am pretty sure that your idea of implementing specialized expressions that stores the SymPy representation would provide a better implementation over what I have write here. However, even with that implementation, I think that it would be better to split the evaluation of arithmetic expressions in a part that could depend on the context (provided by the Evaluation object) and a part that is independent of the context. This is what this or is about. Then you could replace part of the implementation with proper calls to SymPy, and use a smarter class hierarchy, but still I think we want to have a way to do arithmetic without passing throw the standard evaluation process.

rocky · 2023-01-28T01:50:28Z

I think that it would be better to split the evaluation of arithmetic expressions in a part that could depend on the context (provided by the Evaluation object) and a part that is independent of the context.

I totally agree. And then this PR can be much much smaller and simpler. Please keep the new tests too - I like those as well!

The other change for now that makes sense is changing the rule with Gudermannian[Infinity] to Gudermannian[DirectedInfinity[1]] and removing the comment above it.

Doing both of these better is something we should keep in mind and do when the opportunity presents itself.

This would be the simplest part of #766, including tests and changes in `Infinity` and `Precision`. Some of the new tests are commented / marked as xfail because require the other changes.

…ts. (#769) This PR is another fix coming from issues found when I was working on #766. In particular, the way in which Mathics handles Precision and Accuracy for real numbers with 0. nominal value. Also, a bug that prevents parsing Real numbers with fixed accuracy was fixed.

mmatera · 2023-03-05T12:02:52Z

@rocky, I was trying to merge this with the current state of master, but I found a lot of conflicts. My plan is to take the tests added here, and move the changes to another new branch, and then, take the part of the code needed to pass the tests. It will take a while.

mmatera · 2023-03-06T12:35:29Z

I completed the rebase, but for some reason, now the performance is lower than in master. Also, I found some extra bugs in non directly related code. I am going to mark them, then propose fixing for them, and then continue over this.

rocky · 2023-03-06T14:20:33Z

I completed the rebase, but for some reason, now the performance is lower than in master. Also, I found some extra bugs in non directly related code. I am going to mark them, then propose fixing for them, and then continue over this.

Small PRs are always appreciated over large ones like this.

This came up during the #766 rebase: `PrecisionForm` didn't have a `Builtin` symbol, and the `Makeboxes` rule didn't take into account the `form` parameter. This PR fixes these two issues.

mmatera · 2023-03-06T14:36:24Z

@rocky, yes, I am slowly splitting this up.

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. --------- Co-authored-by: rocky <[email protected]> Co-authored-by: R. Bernstein <[email protected]>

…ving unused imports. adding symbols to mathics.core.systemsymbols

mmatera commented Jan 27, 2023

View reviewed changes

rocky reviewed Jan 28, 2023

View reviewed changes

mmatera mentioned this pull request Jan 28, 2023

Improve infinity and Precision #767

Merged

mmatera force-pushed the arithmetic_refactor branch from f5ab2cc to 11eb5ff Compare January 29, 2023 00:15

mmatera mentioned this pull request Jan 30, 2023

More compatibility improvements in Accuracy and Precision. Adding tests. #769

Merged

rocky force-pushed the arithmetic_refactor branch from 11eb5ff to f5ab2cc Compare March 5, 2023 10:05

mmatera force-pushed the arithmetic_refactor branch from f5ab2cc to 10c9cbd Compare March 6, 2023 12:29

mmatera force-pushed the arithmetic_refactor branch from 10c9cbd to c982e6e Compare March 6, 2023 12:37

mmatera mentioned this pull request Mar 6, 2023

fix MakeBoxes[PrecedenceForm[...],form] #808

Merged

mmatera mentioned this pull request Mar 6, 2023

Improve directed infinity #809

Merged

mmatera force-pushed the arithmetic_refactor branch from 2f8c02e to bd3bcc2 Compare March 23, 2023 12:08

mmatera changed the base branch from master to arithmetic_test_zero May 26, 2023 18:18

mmatera added 3 commits May 26, 2023 15:24

flake8 mathics.eval.parts

757011f

Merge branch 'arithmetic_test_zero' into arithmetic_refactor

d5be5ae

Pi is a Numpy constant

7b6f650

mmatera force-pushed the arithmetic_test_zero branch from 757011f to 7b6f650 Compare May 26, 2023 18:27

mmatera added 3 commits May 26, 2023 15:27

Merge branch 'arithmetic_test_zero' into arithmetic_refactor

f39e64e

adding session.parse method, which is handy for low-level tests. remo…

f44dc77

…ving unused imports. adding symbols to mathics.core.systemsymbols

Pi is a Numpy constant

2481118

mmatera force-pushed the arithmetic_test_zero branch from 7b6f650 to 2481118 Compare May 26, 2023 18:53

mmatera added 2 commits May 26, 2023 15:55

Merge branch 'tidy_up_stuff' into arithmetic_test_zero

d9a961a

Merge branch 'arithmetic_test_zero' into arithmetic_refactor

4cf85ac

Base automatically changed from arithmetic_test_zero to tidy_up_stuff May 27, 2023 06:38

Base automatically changed from tidy_up_stuff to master May 27, 2023 11:59

mmatera added 2 commits May 27, 2023 13:08

Merge branch 'master' into arithmetic_refactor

279ae2c

flake8

a0b5310

mmatera force-pushed the arithmetic_refactor branch from e8e0de7 to a0b5310 Compare May 27, 2023 16:21

mmatera and others added 7 commits July 15, 2023 10:13

Merge branch 'master' into arithmetic_refactor

74e5823

Correct typo in error message pattern

a2ada0b

RealAbs and RealSign

87316dc

Merge branch 'master' into RealAbs_and_RealSign

fc54766

merge

b3119c5

tiny tweaks in documentation

4d8f5b1

Merge branch 'RealAbs_and_RealSign' into arithmetic_refactor

c32daa2

mmatera force-pushed the arithmetic_refactor branch from 2d2a37a to c32daa2 Compare July 21, 2023 01:05

merge

261ba42

mmatera mentioned this pull request Jul 21, 2023

refactor arithmetic power #886

Draft

mmatera added 2 commits July 20, 2023 23:11

remove duplicated

c16e3bc

Merge branch 'master' into arithmetic_refactor

539a29b

rocky closed this Aug 4, 2024

rocky deleted the arithmetic_refactor branch August 4, 2024 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arithmetic refactor #766

Arithmetic refactor #766

mmatera commented Jan 27, 2023

mmatera Jan 27, 2023

rocky Jan 28, 2023

mmatera Jan 27, 2023

rocky Jan 28, 2023 •

edited

Loading

mmatera Jan 28, 2023

rocky Jan 28, 2023

rocky commented Jan 28, 2023 •

edited

Loading

rocky commented Jan 28, 2023 •

edited

Loading

rocky Jan 28, 2023

mmatera Jan 28, 2023

mmatera Jan 28, 2023

rocky Jan 28, 2023

mmatera Jan 28, 2023 •

edited

Loading

rocky Jan 28, 2023 •

edited

Loading

mmatera Jan 28, 2023

rocky Jan 28, 2023

mmatera Jan 28, 2023

rocky Jan 28, 2023 •

edited

Loading

rocky Jan 28, 2023

rocky Jan 28, 2023 •

edited

Loading

mmatera Jan 28, 2023

rocky Jan 28, 2023

rocky Jan 28, 2023

mmatera commented Jan 28, 2023

rocky Jan 28, 2023

rocky commented Jan 28, 2023 •

edited

Loading

rocky commented Jan 28, 2023 •

edited

Loading

mmatera commented Jan 28, 2023

rocky commented Jan 28, 2023 •

edited

Loading

mmatera commented Mar 5, 2023

mmatera commented Mar 6, 2023

rocky commented Mar 6, 2023

mmatera commented Mar 6, 2023

		}


		# Here I used the convention of calling eval_* to functions that can produce a new expression, or None

		RealOne = Real(1.0)


		# Here we store numerical constants with their approximate

		return result


		def distribute_factor(expr: BaseElement, factor: BaseElement) -> BaseElement:

Arithmetic refactor #766

Arithmetic refactor #766

Conversation

mmatera commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky commented Jan 28, 2023 • edited Loading

rocky commented Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatera Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

rocky Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rocky Jan 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatera commented Jan 28, 2023

Choose a reason for hiding this comment

rocky commented Jan 28, 2023 • edited Loading

rocky commented Jan 28, 2023 • edited Loading

mmatera commented Jan 28, 2023

rocky commented Jan 28, 2023 • edited Loading

mmatera commented Mar 5, 2023

mmatera commented Mar 6, 2023

rocky commented Mar 6, 2023

mmatera commented Mar 6, 2023

rocky Jan 28, 2023 •

edited

Loading

rocky commented Jan 28, 2023 •

edited

Loading

rocky commented Jan 28, 2023 •

edited

Loading

mmatera Jan 28, 2023 •

edited

Loading

rocky Jan 28, 2023 •

edited

Loading

rocky Jan 28, 2023 •

edited

Loading

rocky Jan 28, 2023 •

edited

Loading

rocky commented Jan 28, 2023 •

edited

Loading

rocky commented Jan 28, 2023 •

edited

Loading

rocky commented Jan 28, 2023 •

edited

Loading