#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: |
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
Commit: | → 9d32f6ad3b3b319b5f3a08b5b933d6fa0cb167ed |
---|
comment:2 Changed 2 years ago by
Status: | new → needs_review |
---|
comment:3 Changed 2 years ago by
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
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
Authors: | → Marius Gerbershagen |
---|
comment:6 follow-up: 9 Changed 2 years ago by
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
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 Changed 2 years ago by
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 Changed 2 years ago by
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
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 follow-up: 12 Changed 2 years ago by
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 Changed 2 years ago by
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
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
Commit: | 9d32f6ad3b3b319b5f3a08b5b933d6fa0cb167ed → 4735f7f005e7be167430979cd38915b18e76d7e9 |
---|
comment:15 Changed 2 years ago by
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
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
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
Status: | needs_work → needs_review |
---|
comment:19 Changed 2 years ago by
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
Commit: | 2d38314c433bc77f3b4f04a66d9429190da45d2c → 629c1d3d53366b234b1a45ba3cd8f7ad20a32f23 |
---|
comment:22 Changed 2 years ago by
Commit: | 629c1d3d53366b234b1a45ba3cd8f7ad20a32f23 → 933bb5a749861d0e0d1c4f513bdb39d7d26b5db6 |
---|
comment:23 Changed 2 years ago by
Status: | needs_work → needs_review |
---|
comment:24 Changed 2 years ago by
Cc: | gh-DaveWitteMorris added |
---|---|
Status: | 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
Commit: | 933bb5a749861d0e0d1c4f513bdb39d7d26b5db6 → 2845ad24760072001715b246fd0096e87303ffb6 |
---|
comment:26 Changed 2 years ago by
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
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
Commit: | 2845ad24760072001715b246fd0096e87303ffb6 → be11386f2d333a4185d8543adda8e5ec0759c393 |
---|
comment:29 follow-ups: 30 31 Changed 23 months ago by
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 Changed 23 months ago by
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 Changed 23 months ago by
Status: | needs_review → positive_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
Branch: | public/bug_convert_symbolic_function_from_maxima → be11386f2d333a4185d8543adda8e5ec0759c393 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:33 Changed 22 months ago by
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
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.
Branch pushed to git repo; I updated commit sha1. New commits:
calculus: fix conversion of symbol functions back from maxima