Skip to content
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

fix tests for sympy conversions #1073

Merged
merged 4 commits into from
Aug 20, 2024
Merged

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Aug 20, 2024

This PR just fixes the pytest, and marks some issues in the conversion of Lambda functions.

rocky and others added 4 commits August 19, 2024 06:47
(These were noticed in working on event tracing)

`mathics/builtin/numbers/algebra.py`: black changes its autoformating

`mathics/builtin/numbers/numbertheory.py`: some linting prefers triples
quotes for docstrings

`mathics/core/convert/sympy.py`: use accurate location for singleton
class. This reduces unnecessary sympy imports

`mathics/core/symbols.py`: shorten prefix and context names for SymPy
variables. In tracing we will see these names a lot. A shorter prefix
makes understanding expressions involving these easier to follow

`CHANGES.rst`: document recent SetEnvironment addition and GetEnvironment
changes

`test/numbers/__init__.py`: needed if test/numbers is tested separately
mathics/builtin/numbers/algebra.py: black changes its autoformatting

mathics/builtin/numbers/numbertheory.py: some linting prefers triples quotes for docstrings

mathics/core/convert/sympy.py: use accurate location for singleton class. This reduces unnecessary sympy imports

CHANGES.rst: document recent SetEnvironment addition and GetEnvironment changes

test/builtin/numbers/__init__.py: needed if testing in this directory is done separately.
@rocky
Copy link
Member

rocky commented Aug 20, 2024

Thanks for doing this! I love the fact that some of the hard-coded context strings are now variables.

I have been thinking about shortening the prefix by removing the "m". That is instead of "mu" and "ms", just "u" and "s".

What strikes me odd about all of this is that basically all SymPy variables or symbols are Mathics3 related. We don't have a way to pass in SymPy variables that are are not Mathics3 related. And just adding any sort of thing before the backtick (which I think is just treated as a character in SymPy) is enough.

What I think we want to "optimize" is the ability to understand how Mathics3 expressions (which generally don't have any context designators) are translated to SymPy and track how these things are propagated and translated.

Adding additional text detracts from where the focus should be by making the correspondence more different than it need be. Adding to the length in debug output also makes harder to read by making long lines of text even longer. It is not the same thing as using longer more explicit variable names.

Your thoughts?

Base automatically changed from misc-tweaks-in-prep-for-event-handler to master August 20, 2024 10:37
@mmatera
Copy link
Contributor Author

mmatera commented Aug 20, 2024

Thanks for doing this! I love the fact that some of the hard-coded context strings are now variables.

I have been thinking about shortening the prefix by removing the "m". That is instead of "mu" and "ms", just "u" and "s".

here what is important is to avoid stepping over names of existing symbols in Sympy. Maybe the underscores and just a letter would be enough for that.

What strikes me odd about all of this is that basically all SymPy variables or symbols are Mathics3 related. We don't have a way to pass in SymPy variables that are are not Mathics3 related. And just adding any sort of thing before the backtick (which I think is just treated as a character in SymPy) is enough.

I guess that the only case where this is not completely true is with slots and anonymous functions. But currently, we are not handling them properly either.

What I think we want to "optimize" is the ability to understand how Mathics3 expressions (which generally don't have any context designators) are translated to SymPy and track how these things are propagated and translated.

Something that I was thinking about is when it makes sense to do the conversion, and how deep the conversion to sympy should go in a nested function. For example, let's say we have something like this:

Sin[Integrate[f[x],{x,a,b}]]

When the integral can be evaluated, then what we actually convert to sympy is its numerical value. On the other hand, if the integral does not have a numerical value, the conversion to Sympy is useless: we know beforehand that after the conversion which is the result.
On the other hand, if the expression is inside another expression like

Series[Sin[Integrate[f[x],{x,a,b}]],{b,0,2}]

then it makes sense to go inside until reaching the atomic parts. Probably we want to take this into account to optimize the conversion.

Adding additional text detracts from where the focus should be by making the correspondence more different than it need be. Adding to the length in debug output also makes harder to read by making long lines of text even longer. It is not the same thing as using longer more explicit variable names.

In debugging, I think that a short prefix can help distinguish what is happening at the level of Sympy and what is at the level of Mathics.

Your thoughts?

@rocky
Copy link
Member

rocky commented Aug 20, 2024

Ok. LGTM - merge when you are satisfied.

@rocky
Copy link
Member

rocky commented Aug 20, 2024

Something that I was thinking about is when it makes sense to do the conversion, and how deep the conversion to sympy should go in a nested function. For example, let's say we have something like this:

Sin[Integrate[f[x],{x,a,b}]]

When the integral can be evaluated, then what we actually convert to sympy is its numerical value. On the other hand, if the integral does not have a numerical value, the conversion to Sympy is useless: we know beforehand that after the conversion which is the result. On the other hand, if the expression is inside another expression like

Series[Sin[Integrate[f[x],{x,a,b}]],{b,0,2}]

then it makes sense to go inside until reaching the atomic parts. Probably we want to take this into account to optimize the conversion.

This kind of thing is typically done as part of a type inference and special "optimize" pass. Over time I think there will be more type information included.

@mmatera mmatera merged commit ef8a0ce into master Aug 20, 2024
7 checks passed
@mmatera mmatera deleted the fix_pytest_for_sympy_conversions branch August 20, 2024 12:18
rocky added a commit that referenced this pull request Aug 29, 2024
This PR just fixes the pytest, and marks some issues in the conversion
of Lambda functions.

---------

Co-authored-by: rocky <[email protected]>
rocky added a commit that referenced this pull request Aug 29, 2024
This PR just fixes the pytest, and marks some issues in the conversion
of Lambda functions.

---------

Co-authored-by: rocky <[email protected]>
rocky added a commit that referenced this pull request Aug 29, 2024
This PR just fixes the pytest, and marks some issues in the conversion
of Lambda functions.

---------

Co-authored-by: rocky <[email protected]>
@rocky
Copy link
Member

rocky commented Sep 1, 2024

@mmatera I was revisiting your example with

Sin[Integrate[f[x],{x,a,b}]]

using the debugger to see what I can learn using the debugger. First I activate tracing for SymPy and then I run this expression:

In[1]:= LoadModule["pymathics.debugger"]
Out[1]= "pymathics.debugger"
In[2]:= TraceActivate[{SymPy->True}]
Out[2]= TraceActivate[{SymPy → True}]
SymPy call  : sin(SympyExpression(_uSystem`Integrate, SympyExpression(_uGlobal`f, _uGlobal`x), SympyExpression(_uSystem`List, _uGlobal`x, _uGlobal`a, _uGlobal`b)),)
SymPy result: sin(SympyExpression(_uSystem`Integrate, SympyExpression(_uGlobal`f, _uGlobal`x), SympyExpression(_uSystem`List, _uGlobal`x, _uGlobal`a, _uGlobal`b)))
SymPy call  : sin(SympyExpression(_uSystem`Integrate, SympyExpression(_uGlobal`f, _uGlobal`x), SympyExpression(_uSystem`List, _uGlobal`x, _uGlobal`a, _uGlobal`b)),)
SymPy result: sin(SympyExpression(_uSystem`Integrate, SympyExpression(_uGlobal`f, _uGlobal`x), SympyExpression(_uSystem`List, _uGlobal`x, _uGlobal`a, _uGlobal`b)))
...

Ok, so now I want to see what is going on with what looks like two duplicate SymPy calls to sin(). So here I switch to go into the debugger instead of tracing:

In[4]:= DebugActivate[{SymPy->True}]
Out[4]= DebugActivate[{SymPy → True}]

And when I now run the expression and look at the Backtrace, I see the duplicate call is coming from the same location in rule replacement do_replace():

In[5]:= Series[Sin[Integrate[f[x],{x,a,b}]],{b,0,2}]
SymPy call  : sin(SympyExpression(_uSystem`Integrate, SympyExpression(_uGlobal`f, _uGlobal`x), SympyExpression(_uSystem`List, _uGlobal`x, _uGlobal`a, _uGlobal`b)),)
(/tmp/Mathics3/mathics-core/mathics/core/builtin.py:575 @232): to_sympy
SP 575                     return tracing.run_sympy(sympy_function, *sympy_args)
(Mathics3 Debug) bt 4
->0 to_sympy(self=<mathics.builtin.numbers.trig.Sin object at 0x7a0820f09130>, expr=<Expression: <Symbol: System...)
     called from file '/tmp/Mathics3/mathics-core/mathics/core/builtin.py' at line 575
##1 expression_to_sympy(expr=<Expression: <Symbol: System`Sin>[<Expression: <Symbol: System`Integrate>[<Expression: <Symbol...)
     called from file '/tmp/Mathics3/mathics-core/mathics/core/convert/sympy.py' at line 235
##2 to_sympy(self=<Expression: <Symbol: System`Sin>[<Expression: <Symbol: System`Integrate>[<Expression: <Symbol...)
     called from file '/tmp/Mathics3/mathics-core/mathics/core/expression.py' at line 1560
##3 eval(self=<mathics.builtin.numbers.trig.Sin object at 0x7a0820f09130>, z=<Expression: <Symbol: System`In...)
     called from file '/tmp/Mathics3/mathics-core/mathics/core/builtin.py' at line 613
(Mathics3 Debug) frame 4
(/tmp/Mathics3/mathics-core/mathics/core/rules.py:269 @226): do_replace
SP 269             return self.function(evaluation=evaluation, **vars_noctx)
(Mathics3 Debug) expression
<Expression: <Symbol: System`Sin>[<Expression: <Symbol: System`Integrate>[<Expression: <Symbol: Global`f>[<Symbol: Global`x>]>, <ListExpression: (<Symbol: Global`x>, <Symbol: Global`a>, <Symbol: Global`b>)>]>]>
(Mathics3 Debug) continue
SymPy result: sin(SympyExpression(_uSystem`Integrate, SympyExpression(_uGlobal`f, _uGlobal`x), SympyExpression(_uSystem`List, _uGlobal`x, _uGlobal`a, _uGlobal`b)))
SymPy call  : sin(SympyExpression(_uSystem`Integrate, SympyExpression(_uGlobal`f, _uGlobal`x), SympyExpression(_uSystem`List, _uGlobal`x, _uGlobal`a, _uGlobal`b)),)
(/tmp/Mathics3/mathics-core/mathics/core/builtin.py:575 @232): to_sympy
SP 575                     return tracing.run_sympy(sympy_function, *sympy_args)
(Mathics3 Debug) bt 4
->0 to_sympy(self=<mathics.builtin.numbers.trig.Sin object at 0x7a0820f09130>, expr=<Expression: <Symbol: System...)
     called from file '/tmp/Mathics3/mathics-core/mathics/core/builtin.py' at line 575
##1 expression_to_sympy(expr=<Expression: <Symbol: System`Sin>[<Expression: <Symbol: System`Integrate>[<Expression: <Symbol...)
     called from file '/tmp/Mathics3/mathics-core/mathics/core/convert/sympy.py' at line 235
##2 to_sympy(self=<Expression: <Symbol: System`Sin>[<Expression: <Symbol: System`Integrate>[<Expression: <Symbol...)
     called from file '/tmp/Mathics3/mathics-core/mathics/core/expression.py' at line 1560
##3 eval(self=<mathics.builtin.numbers.trig.Sin object at 0x7a0820f09130>, z=<Expression: <Symbol: System`In...)
     called from file '/tmp/Mathics3/mathics-core/mathics/core/builtin.py' at line 613
(Mathics3 Debug) frame 4
(/tmp/Mathics3/mathics-core/mathics/core/rules.py:269 @226): do_replace
SP 269             return self.function(evaluation=evaluation, **vars_noctx)
(Mathics3 Debug) expression
<Expression: <Symbol: System`Sin>[<Expression: <Symbol: System`Integrate>[<Expression: <Symbol: Global`f>[<Symbol: Global`x>]>, <ListExpression: (<Symbol: Global`x>, <Symbol: Global`a>, <Symbol: Global`b>)>]>]>
(Mathics3 Debug) info frame
Frame 4
  current line number: 269
  last instruction: 226
  code: <code object do_replace at 0x7a082a2163a0, file "/tmp/Mathics3/mathics-core/mathics/core/rules.py", line 254>
  previous frame: <frame at 0x7a081ee58580, file '/tmp/Mathics3/mathics-core/mathics/core/rules.py', line 78, code yield_match>
  tracing function: None
(Mathics3 Debug) 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants