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

[spaghettisalat]

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 #14608 comment:9 and links therein for more discussion about this issue.

CC: @nbruin @egourgoulhon @rwst @DaveWitteMorris

Component: symbolics

Author: Marius Gerbershagen

Branch: be11386

Reviewer: Eric Gourgoulhon

Issue created by migration from https://trac.sagemath.org/ticket/31047

[sagetrac-git]

Commit: 9d32f6a

[sagetrac-git]

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

9d32f6a

calculus: fix conversion of symbol functions back from maxima

[egourgoulhon]

comment:3

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.

[egourgoulhon]

comment:4

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

[kliem]

Author: Marius Gerbershagen

[sagetrac-tmonteil]

comment:6

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 ?

[nbruin]

comment:7

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.

[egourgoulhon]

comment:8

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?

[egourgoulhon]

comment:9

Replying to @sagetrac-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)

[spaghettisalat]

comment:10

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

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

[spaghettisalat]

comment:11

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.

[egourgoulhon]

comment:12

Replying to @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.

[sagetrac-tmonteil]

comment:13

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

[sagetrac-git]

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

6ffb8c5	sage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
4735f7f	add tests for Trac #31047

[sagetrac-git]

Changed commit from 9d32f6a to 4735f7f

[spaghettisalat]

comment:15

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.

[mkoeppe]

comment:16

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

[sagetrac-git]

Changed commit from 4735f7f to 98ac37d

[mkoeppe]

comment:21

needs rebase

[sagetrac-git]

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

38aa89e	sage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
d6a1f0a	add tests for Trac #31047
933bb5a	sympy interface: fix conversion of symbolic functions from sympy

[sagetrac-git]

Changed commit from 629c1d3 to 933bb5a

[mkoeppe]

comment:24

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.

[sagetrac-git]

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

d3ee00d	sage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
15ad274	add tests for Trac #31047
2845ad2	sympy interface: fix conversion of symbolic functions from sympy

[sagetrac-git]

Changed commit from 933bb5a to 2845ad2

[spaghettisalat]

comment:26

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?

[egourgoulhon]

Reviewer: Eric Gourgoulhon

[egourgoulhon]

comment:27

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.

[sagetrac-git]

Changed commit from 2845ad2 to be11386

[sagetrac-git]

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

f68e82f	sage.calculus.calculus: simplify handling of variables and symbolic functions during parsing
607c365	add tests for Trac #31047
be11386	sympy interface: fix conversion of symbolic functions from sympy

[spaghettisalat]

comment:29

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.)

[mkoeppe]

comment:30

Replying to @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.

[egourgoulhon]

comment:31

Replying to @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.

[vbraun]

Changed branch from public/bug_convert_symbolic_function_from_maxima to be11386

[mwageringel]

comment:33

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

[mwageringel]

Changed commit from be11386 to none

[mwageringel]

comment:34

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.