Opened 2 years ago

Closed 23 months ago

Last modified 22 months ago

#31047 closed defect (fixed)

Conversion of symbolic functions with latex_name or nargs from maxima and sympy is broken

Reported by: gh-spaghettisalat Owned by:
Priority: critical Milestone: sage-9.3
Component: symbolics Keywords:
Cc: nbruin, egourgoulhon, rws, gh-DaveWitteMorris Merged in:
Authors: Marius Gerbershagen Reviewers: Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: be11386 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

When converting a NewSymbolicFunction to a maxima expression and back, sage sometimes returns the correct symbolic function and sometimes it creates a new function with the same name. This happens only when the function has additional information (i.e. a latex_name or nargs) attached to it. For example:

var('phi')
assume(phi >= 0)
function('Cp', latex_name='C_+')
Cp(phi).simplify_full() # convert to maxima and back

returns an expression with a new Cp function which is not equal to the original one and which doesn't have a latex name, but only if the assume(phi >= 0) happens before defining the function.

The issue is that the conversion functions hold a local copy of the symbol table which is not kept in sync with the actual symbol table that new functions are added to. If the conversion functions do not find the function by name in the local copy, function_factory is invoked which checks for both name, latex_name and nargs when looking for already registered functions. Since of course the maxima expression doesn't include the latex_name, it doesn't find the already registered function and creates a new one.

I have implemented a fix which checks both the local and global copies of the symbol table, but I'm not sure this is the right way to fix things. First, it is not clear to me why the conversion functions have a local copy of the symbol table in the first place. Second, it makes no sense to me that function_factory looks for a matching latex_name when registering a new function. I see no use case for having to functions with the same name and different latex_name registered. After all, if I type in the name of the function in the sagemath prompt, I will only get the second definition which I typed in and thus I can't use the first definition anyway.

See also https://trac.sagemath.org/ticket/14608#comment:9 and links therein for more discussion about this issue.

Change History (34)

comment:1 Changed 2 years ago by git

Commit: 9d32f6ad3b3b319b5f3a08b5b933d6fa0cb167ed

Branch pushed to git repo; I updated commit sha1. New commits:

9d32f6acalculus: fix conversion of symbol functions back from maxima

comment:2 Changed 2 years ago by gh-spaghettisalat

Status: newneeds_review

comment:3 Changed 2 years ago by egourgoulhon

Thank you very much for taking care of this long standing issue!

I've performed a few tests and the fix seems good to me. Let us wait for some other viewpoints before setting a positive review. Also it would be nice if the patchbot could run on this ticket branch.

comment:4 Changed 2 years ago by egourgoulhon

Another issue with symbolic functions is #27492. It might also be related to the way symbolic functions are stored in the symbol table.

comment:5 Changed 2 years ago by gh-kliem

Authors: Marius Gerbershagen

comment:6 Changed 2 years ago by tmonteil

Status: needs_reviewneeds_work

I am not able to judge the quality of the fix, but you should at least add doctests. Eric, could you please provide the tests you did so that they can be added as well ?

comment:7 Changed 2 years ago by nbruin

I can comment on one question in the description: since it is possible to do the following:

sage: f1=sage.symbolic.function_factory.function('f',latex_name="\phi")
sage: f2=sage.symbolic.function_factory.function('f',latex_name="\psi")
sage: f1(x)-f2(x)
f(x) - f(x)
sage: latex(f1(x)-f2(x))
\phi\left(x\right) - \psi\left(x\right)

I think our hand is forced on taking into account LaTeX names. It may not be advisable to create many functions with the same print name, it's not forbidden. In fact, automatic code could easily create many functions, and they should not interfere with the latex names used elsewhere.

You're probably going to run into problems if you do this kind of stuff in other interfaces (particularly maxima), though.

comment:8 in reply to:  7 Changed 2 years ago by egourgoulhon

Replying to nbruin:

I think our hand is forced on taking into account LaTeX names. It may not be advisable to create many functions with the same print name, it's not forbidden.

It should be forbidden, IMHO. As pointed out in the ticket description, I don't see any use case for such a feature. Since maxima has no concept of LaTeX name and maxima is currently heavily used for simplifications in Sage symbolic calculus, it would be safe to forbid to declare a function with an already existing name but a different LaTeX name.

You're probably going to run into problems if you do this kind of stuff in other interfaces (particularly maxima), though.

What do you mean by "other interfaces (particularly maxima)", given that this ticket is about the maxima interface?

comment:9 in reply to:  6 Changed 2 years ago by egourgoulhon

Replying to tmonteil:

I am not able to judge the quality of the fix, but you should at least add doctests. Eric, could you please provide the tests you did so that they can be added as well ?

Here is a doctest adapted from the ticket description:

sage: assume(x > 0)                                                        
sage: Cp = function('Cp', latex_name=r'C_+') 
sage: s = Cp(x).simplify()                 
sage: s - Cp(x)  # in Sage 9.2, returns Cp(x) - Cp(x)                                                             
0
sage: latex(s)  # in Sage 9.2, returns {\rm Cp}\left(x\right)                                                                    
C_+\left(x\right)

comment:10 Changed 2 years ago by gh-spaghettisalat

Summary: Conversion of symbolic functions from maxima is brokenConversion of symbolic functions with latex_name or nargs from maxima and sympy is broken

The sympy interfaces suffers from exactly the same problem, for example in

var('phi')
function('Cp', latex_name='C_+')
Cp(phi)._sympy_()._sage_()

comment:11 in reply to:  7 ; Changed 2 years ago by gh-spaghettisalat

Replying to nbruin:

I think our hand is forced on taking into account LaTeX names. It may not be advisable to create many functions with the same print name, it's not forbidden. In fact, automatic code could easily create many functions, and they should not interfere with the latex names used elsewhere.

But in basically all cases, sagemath already assumes that the print name can be treated as a unique identifier for the function. The documentation never mentions anything else. External interfaces rely on the assumption. The sagemath prompt treats the print name as a unique identifier: when I create two functions with the same print name and two different latex names and then type in the function name in the prompt, I get only one of the two functions.

Therefore, the "feature" that one can create two functions with the same print name but different latex names is in my opinion first of all profoundly useless (because most of the stuff one would want to do with these functions won't work properly) and secondly serves only as a pitfall to confuse unsuspecting users.

Given that the sympy interface is also broken (it is not unlikely that other interfaces may also suffer from the same problem), in my opinion the only sane solution is to patch function_factory to take into account only the print name.

comment:12 in reply to:  11 Changed 2 years ago by egourgoulhon

Replying to gh-spaghettisalat:

Therefore, the "feature" that one can create two functions with the same print name but different latex names is in my opinion first of all profoundly useless (because most of the stuff one would want to do with these functions won't work properly) and secondly serves only as a pitfall to confuse unsuspecting users.

+1

Given that the sympy interface is also broken (it is not unlikely that other interfaces may also suffer from the same problem), in my opinion the only sane solution is to patch function_factory to take into account only the print name.

Yes, this seems the route to go.

comment:13 Changed 2 years ago by tmonteil

In any case, there is something weird about the way some kind of "unique representation" for functions (and variables) is handled (both with and without the branch):

sage: f = function('f', nargs=2)
sage: f(1,2)
f(1, 2)
sage: g = function('f', nargs=1)
sage: f(1,2)
TypeError: Symbolic function f takes exactly 1 arguments (2 given)
sage: f is g
True

comment:14 Changed 2 years ago by git

Commit: 9d32f6ad3b3b319b5f3a08b5b933d6fa0cb167ed4735f7f005e7be167430979cd38915b18e76d7e9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6ffb8c5sage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
4735f7fadd tests for Trac #31047

comment:15 Changed 2 years ago by gh-spaghettisalat

Status: needs_workneeds_review

I have implemented a basic fix to the specific problem reported on this ticket, i.e. only for the conversion of symbolic functions from maxima. The inconsistent behaviour of function_factory is untouched because I can't be bothered to wade through even more piles of spaghetti code to fix that too. The sympy interface is also still broken because for some weird reason the conversion functions between sympy and sage seem to be partially duplicated in both projects and I don't know where to implement the fix (in sage, sympy or both), although the fix itself would be very simple.

comment:16 Changed 2 years ago by mkoeppe

Status: needs_reviewneeds_work

patchbot reports:

sage -t --long --warn-long 69.4 --random-seed=0 src/sage/functions/log.py  # 1 doctest failed
sage -t --long --warn-long 69.4 --random-seed=0 src/sage/interfaces/sympy.py  # 2 doctests failed

comment:17 Changed 2 years ago by git

Commit: 4735f7f005e7be167430979cd38915b18e76d7e998ac37dfe44bffb960a789c9419f70b462d03d22

Branch pushed to git repo; I updated commit sha1. New commits:

98ac37dsympy interface: fix conversion of symbolic functions from sympy

comment:18 Changed 2 years ago by gh-spaghettisalat

Status: needs_workneeds_review

comment:19 Changed 2 years ago by git

Commit: 98ac37dfe44bffb960a789c9419f70b462d03d222d38314c433bc77f3b4f04a66d9429190da45d2c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2d38314sympy interface: fix conversion of symbolic functions from sympy

comment:20 Changed 2 years ago by git

Commit: 2d38314c433bc77f3b4f04a66d9429190da45d2c629c1d3d53366b234b1a45ba3cd8f7ad20a32f23

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bc62cdbsage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
26b48f2add tests for Trac #31047
629c1d3sympy interface: fix conversion of symbolic functions from sympy

comment:21 Changed 2 years ago by mkoeppe

Status: needs_reviewneeds_work

needs rebase

comment:22 Changed 2 years ago by git

Commit: 629c1d3d53366b234b1a45ba3cd8f7ad20a32f23933bb5a749861d0e0d1c4f513bdb39d7d26b5db6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

38aa89esage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
d6a1f0aadd tests for Trac #31047
933bb5asympy interface: fix conversion of symbolic functions from sympy

comment:23 Changed 2 years ago by gh-spaghettisalat

Status: needs_workneeds_review

comment:24 Changed 2 years ago by mkoeppe

Cc: gh-DaveWitteMorris added
Status: needs_reviewneeds_work

Just a superficial comment (because I don't know this part of the code very well): All functions (including internal functions) need a docstring and tests to conform with our coding style.

comment:25 Changed 2 years ago by git

Commit: 933bb5a749861d0e0d1c4f513bdb39d7d26b5db62845ad24760072001715b246fd0096e87303ffb6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d3ee00dsage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
15ad274add tests for Trac #31047
2845ad2sympy interface: fix conversion of symbolic functions from sympy

comment:26 Changed 2 years ago by gh-spaghettisalat

Status: needs_workneeds_review

Just a superficial comment (because I don't know this part of the code very well): All functions (including internal functions) need a docstring and tests to conform with our coding style.

Everything I added is already covered with tests, either the ones I added in this ticket or existing tests for other functionality that depends on the stuff I changed. Therefore I added only some docstrings. I hope that is finally enough to get this merged.

Is there anybody more familiar with this part of sagemath who we could cc to review this?

comment:27 Changed 23 months ago by egourgoulhon

Reviewers: Eric Gourgoulhon

I've performed a few tests and everything seems OK. Thanks for the fix!

Regarding :comment:24, the class _Function_swap_harmonic in src/sage/functions/log.py should have some (minimal) doctests, as well as the function _sympysage_function_by_name and the class UndefSageHelper in src/sage/interfaces/sympy.py. This will make the coverage plugin of the patchbot happy. Once this is made, I think we can set the ticket to positive review.

comment:28 Changed 23 months ago by git

Commit: 2845ad24760072001715b246fd0096e87303ffb6be11386f2d333a4185d8543adda8e5ec0759c393

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f68e82fsage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
607c365add tests for Trac #31047
be11386sympy interface: fix conversion of symbolic functions from sympy

comment:29 Changed 23 months ago by gh-spaghettisalat

I have copied some of the tests to the new helper functions created in the new commits.

(Not that this would make any difference since as I said before, there were already tests for the functionality and all I've done now is to spread the tests out into more places. I don't want to be too rude here, but just counting whether a function has doctests or not seems like a pretty shitty way to measure test coverage.)

comment:30 in reply to:  29 Changed 23 months ago by mkoeppe

Replying to gh-spaghettisalat:

just counting whether a function has doctests or not seems like a pretty shitty way to measure test coverage.

Hard to disagree with this, but we don't have a better way.

comment:31 in reply to:  29 Changed 23 months ago by egourgoulhon

Status: needs_reviewpositive_review

Replying to gh-spaghettisalat:

I have copied some of the tests to the new helper functions created in the new commits.

OK, this provides only indirect doctests, but let's move on in order to have the fix merged in 9.3!

(Not that this would make any difference since as I said before, there were already tests for the functionality and all I've done now is to spread the tests out into more places. I don't want to be too rude here, but just counting whether a function has doctests or not seems like a pretty shitty way to measure test coverage.)

Beside testing, another virtue of doctests is to illustrate quickly the use of the function; this is useful even for helper functions, i.e. for functions that only developers are supposed to take a look at.

comment:32 Changed 23 months ago by vbraun

Branch: public/bug_convert_symbolic_function_from_maximabe11386f2d333a4185d8543adda8e5ec0759c393
Resolution: fixed
Status: positive_reviewclosed

comment:33 Changed 22 months ago by gh-mwageringel

Commit: be11386f2d333a4185d8543adda8e5ec0759c393

This ticket causes an issue with conversions in the Mathematica interface, see #31756. Do you have an idea that might solve this?

comment:34 Changed 22 months ago by gh-mwageringel

An unrelated comment on this change:

-def symbolic_expression_from_string(s, syms=None, accept_sequence=False):
+def symbolic_expression_from_string(s, syms={}, accept_sequence=False):

Usually it is best to avoid using {} as default value, since it is mutable, so the value that is used as default for all calls to the function can change.

The way it is used in this particular case does not seem to make a problem, though, as the value is not mutated inside the function.

Note: See TracTickets for help on using tickets.