Opened 2 years ago

Closed 23 months ago

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

Reported by: Owned by: gh-spaghettisalat critical sage-9.3 symbolics nbruin, egourgoulhon, rws, gh-DaveWitteMorris Marius Gerbershagen Eric Gourgoulhon N/A be11386

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

### comment:1 Changed 2 years ago by git

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

 ​9d32f6a calculus: fix conversion of symbol functions back from maxima

### comment:2 Changed 2 years ago by gh-spaghettisalat

Status: new → needs_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 follow-up:  9 Changed 2 years ago by tmonteil

Status: needs_review → needs_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 follow-ups:  8  11 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

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

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 broken → Conversion 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 ; follow-up:  12 Changed 2 years ago by gh-spaghettisalat

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

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

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

### comment:15 Changed 2 years ago by gh-spaghettisalat

Status: needs_work → needs_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_review → needs_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: 4735f7f005e7be167430979cd38915b18e76d7e9 → 98ac37dfe44bffb960a789c9419f70b462d03d22

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

 ​98ac37d sympy interface: fix conversion of symbolic functions from sympy

### comment:18 Changed 2 years ago by gh-spaghettisalat

Status: needs_work → needs_review

### comment:19 Changed 2 years ago by git

Commit: 98ac37dfe44bffb960a789c9419f70b462d03d22 → 2d38314c433bc77f3b4f04a66d9429190da45d2c

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

 ​2d38314 sympy interface: fix conversion of symbolic functions from sympy

### comment:20 Changed 2 years ago by git

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

 ​bc62cdb sage.calculus.calculus: simplify handling of variables and symbolic functions during parsing ​26b48f2 add tests for Trac #31047 ​629c1d3 sympy interface: fix conversion of symbolic functions from sympy

### comment:21 Changed 2 years ago by mkoeppe

Status: needs_review → needs_work

needs rebase

### comment:22 Changed 2 years ago by 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

### comment:23 Changed 2 years ago by gh-spaghettisalat

Status: needs_work → needs_review

### comment:24 Changed 2 years ago by mkoeppe

Cc: gh-DaveWitteMorris added needs_review → needs_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

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

### comment:26 Changed 2 years ago by gh-spaghettisalat

Status: needs_work → needs_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

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

### comment:29 follow-ups:  30  31 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

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_review → positive_review

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_maxima → be11386f2d333a4185d8543adda8e5ec0759c393 → fixed positive_review → closed

### comment:33 Changed 22 months ago by gh-mwageringel

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.