Opened 9 years ago
Closed 5 years ago
#8734 closed defect (fixed)
make sage variables unique in maxima
Reported by:  jason  Owned by:  jason 

Priority:  major  Milestone:  sage6.3 
Component:  interfaces  Keywords:  
Cc:  was, acleone, kcrisman, robert.marik, burcin, saliola  Merged in:  
Authors:  Jason Grout, Ralf Stephan  Reviewers:  Volker Braun, Paul Zimmermann, KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  7a6696b (Commits)  Commit:  7a6696b714e638461cf4f77adbb65ecf413a2e4e 
Dependencies:  #6882  Stopgaps: 
Description
This patch prepends a unique string, "_SAGE_VAR_", to each variable name in maxima, to avoid conflicts with existing variables in maxima.
Attachments (1)
Change History (69)
Changed 9 years ago by
comment:1 Changed 9 years ago by
 Cc was acleone added
 Status changed from new to needs_work
comment:2 Changed 9 years ago by
 Cc kcrisman added
I think there might also be another ticket about this somewhere...
comment:3 Changed 9 years ago by
Hi, it is also #8634 and there is a dicussion at sagedevel
comment:4 Changed 9 years ago by
 Cc robert.marik added
comment:5 followup: ↓ 6 Changed 9 years ago by
 Owner changed from was to jason
This should fix #6882
comment:6 in reply to: ↑ 5 Changed 9 years ago by
comment:7 Changed 9 years ago by
 Cc burcin added
comment:8 Changed 9 years ago by
 Cc saliola added
comment:9 Changed 9 years ago by
This ticket may also fix #9538.
comment:10 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:11 Changed 6 years ago by
 Work issues set to rebase
patch should be rebased, it fails to apply to Sage 5.11:
sage: hg_sage.import_patch("/tmp/trac8734maximavars.patch") cd "/home/zimmerma/Downloads/sage5.11/devel/sage" && sage hg import "/tmp/trac8734maximavars.patch" applying /tmp/trac8734maximavars.patch patching file sage/calculus/calculus.py Hunk #1 FAILED at 1450 Hunk #2 FAILED at 1461 2 out of 2 hunks FAILED  saving rejects to file sage/calculus/calculus.py.rej patching file sage/symbolic/assumptions.py Hunk #1 FAILED at 100 1 out of 1 hunks FAILED  saving rejects to file sage/symbolic/assumptions.py.rej abort: patch failed to apply
Paul
comment:12 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:13 Changed 5 years ago by
 Branch set to u/rws/ticket/8734
 Modified changed from 01/30/14 21:20:52 to 01/30/14 21:20:52
comment:14 Changed 5 years ago by
 Commit set to 87d944ffbe59afe29de2faea2b569f7eaa904295
 Reviewers set to Ralf Stephan
Rebased on 6.2.beta3
Patch doesn't seem ready:
sage t long src/sage/symbolic/integration/integral.py # 3 doctests failed sage t long src/sage/symbolic/assumptions.py # 26 doctests failed sage t long src/sage/symbolic/pynac.pyx # 1 doctest failed sage t long src/sage/symbolic/expression.pyx # 21 doctests failed sage t long src/sage/calculus/desolvers.py # 63 doctests failed sage t long src/sage/calculus/calculus.py # 13 doctests failed sage t long src/sage/calculus/functional.py # 1 doctest failed
New commits:
87d944f  Trac #8734: Make sage variables unique in maxima.

comment:15 Changed 5 years ago by
 Commit changed from 87d944ffbe59afe29de2faea2b569f7eaa904295 to 0ee7f20ff6a003d52224c84510e667483c6ddc2b
comment:16 Changed 5 years ago by
 Reviewers Ralf Stephan deleted
 Work issues changed from rebase to fix doctest problems
This fixes the errors in symbolic. More to come...
comment:17 Changed 5 years ago by
Thanks for working on this! This could also then eventually lead to a way to deal with the extra variables that come from e.g. differential equation solvers in Maxima  if we picked up a nonexistent (in Sage) variable, we could perhaps do something with it. (There are tickets about this out there, so I won't repeat that discussion.)
comment:18 Changed 5 years ago by
 Commit changed from 0ee7f20ff6a003d52224c84510e667483c6ddc2b to a5f27eed1dfb83e1c979ecb23ad2854ccd00400b
comment:19 Changed 5 years ago by
 Status changed from needs_work to needs_review
This fixes the last doctests, mainly by handling the desolver entry points.
comment:20 Changed 5 years ago by
Merge conflict, please merge in the latest beta
comment:21 Changed 5 years ago by
 Branch changed from u/rws/ticket/8734 to u/rws/ticket/87341
 Commit changed from a5f27eed1dfb83e1c979ecb23ad2854ccd00400b to f00cd8d5dc695dee49f59164251772db46bf48c3
 Work issues fix doctest problems deleted
New commits:
3d90c49  Trac #8734: Make sage variables unique in maxima.

a6cbf80  factor out missing assumption error handling; filter _SAGE_VAR_

3f24835  fix typo leading to error

f36c6b5  two further maxima calls adapted; two more doctests fixed

ced268d  fix and generalize missing_assumption(); resp. doctest adaptions

a74ec00  add even more cases to add _SAGE_VAR_; fix doctests

f00cd8d  Merge branch 'rev/8734' into tmp

comment:22 Changed 5 years ago by
 Reviewers set to Volker Braun
comment:23 Changed 5 years ago by
sage t long src/sage/functions/bessel.py # 3 doctests failed sage t long src/sage/interfaces/maxima_abstract.py # 16 doctests failed sage t long src/sage/tests/french_book/recequadiff.py # 1 doctest failed sage t long src/sage/interfaces/maxima.py # 2 doctests failed sage t long src/doc/en/constructions/calculus.rst # 2 doctests failed sage t long src/sage/interfaces/maxima_lib.py # 11 doctests failed sage t long src/sage/functions/other.py # 4 doctests failed sage t long src/sage/functions/orthogonal_polys.py # 5 doctests failed sage t long src/sage/symbolic/expression_conversions.py # 2 doctests failed sage t long src/sage/interfaces/interface.py # 10 doctests failed
comment:24 Changed 5 years ago by
 Status changed from needs_review to needs_work
comment:25 Changed 5 years ago by
 Commit changed from f00cd8d5dc695dee49f59164251772db46bf48c3 to be9367f966fc76ba6028b57ab4317530cfd1205e
comment:26 followup: ↓ 27 Changed 5 years ago by
Concerning the remaining doctests in interfaces/maxima_abstract.py
I am at a loss at what to do. For example this
sage: x,y = var('x,y'); f = maxima.function('x','sin(x)') sage: type(f) <class 'sage.interfaces.maxima.MaximaElementFunction'> sage: g=maxima.cos(x) sage: type(g) <class 'sage.interfaces.maxima.MaximaElement'> sage: f+g cos(_SAGE_VAR_x)+sin(x) sage: (f+g)(2) cos(_SAGE_VAR_x)+sin(2)
shows that while the MaximaElementFunction f
has the variable x
which is associated with _SAGE_VAR_x
in Maxima (and calling the function works as expected), the MaximaElement g
shows _SAGE_VAR_x
which has of course no Maxima pendant (and calling the function bombs). Naively both should behave identically.
sage: h=SR(maxima.cos(x)) sage: h cos(x) sage: h(2) cos(2) sage: f+h cos(_SAGE_VAR_x)+sin(x) sage: (f+h)(2) cos(_SAGE_VAR_x)+sin(2)
Moreover, if g
gets converted to SR
it behaves fine but when converted to type(f)
by using it as rhs it gets _SAGE_VAR_x
as parameter. What is the next step?
comment:27 in reply to: ↑ 26 Changed 5 years ago by
As far as I understand now, a MaximaElementFunction
is a function defined within the Maxima process, and so x
is the parameter name, not a registered variable. Thus the output sin(x)
makes sense. OTOH any _SAGE_VAR_x
that appears in a MaximaElement
repr refers to a registered variable within Maxima that is associated with a registered Sage var named x
. Let's interpret the above output in the light of this.
sage: f+g cos(_SAGE_VAR_x)+sin(x)
Correct but can the user make sense of it?
sage: (f+g)(2) cos(_SAGE_VAR_x)+sin(2)
That is not less correct than
sage: (sin(x)+cos(y))(2) cos(y) + sin(2)
and it just seems to be another case of SR._call_element_()
where is stated: "Note that you make get unexpected results when calling symbolic expressions and not explicitly giving the variables."
Moreover, it was a hack that this worked at all in the MaximaAbstractElementFunction._add_()
doctests because
sage: f = maxima.function('z','sin(z)') sage: f sin(z) sage: f(2) sin(2) sage: (sin(z))(2)  NameError Traceback (most recent call last) NameError: name 'z' is not defined
comment:28 Changed 5 years ago by
 Commit changed from be9367f966fc76ba6028b57ab4317530cfd1205e to 06b952805ab10b7f30b4c06c1fcd487cbf1b7dce
comment:29 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:30 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:31 Changed 5 years ago by
 Commit changed from 06b952805ab10b7f30b4c06c1fcd487cbf1b7dce to 6fffc21fbfe9a72f40f51ba22efec6dd9ec28ac5
Branch pushed to git repo; I updated commit sha1. New commits:
6fffc21  Merge branch 'develop' into t/8734/ticket/87341

comment:32 Changed 5 years ago by
 Commit changed from 6fffc21fbfe9a72f40f51ba22efec6dd9ec28ac5 to 3d74cad55ddc6d1a0c64356a59c88cdac1a6f2d9
Branch pushed to git repo; I updated commit sha1. New commits:
3d74cad  8734: rename private function, doctest it

comment:33 Changed 5 years ago by
 Commit changed from 3d74cad55ddc6d1a0c64356a59c88cdac1a6f2d9 to cec9681b1ebb379ed6eccec389981512acd527cb
comment:34 Changed 5 years ago by
 Commit changed from cec9681b1ebb379ed6eccec389981512acd527cb to f70208835cb64305fc90524a9b67c5a86aed0c1e
Branch pushed to git repo; I updated commit sha1. New commits:
f702088  take back unrelated change

comment:35 Changed 5 years ago by
 Reviewers changed from Volker Braun to Volker Braun, Paul Zimmerman, KarlDieter Crisman
Hi! Thanks for keeping at this. I'll try to look at it one final time soon  waiting for the latest beta to compile. In the meantime:
I like the renaming and doctesting of the missing assumption function, and I agree with your analysis of the Maxima 'parameters', as you call them  yes, those are completely separate from Sage. I would encourage you to be even more explicit than
The parameter ``x`` is different from the symbolic variable::
by saying something about it being a Maxima parameter versus Sage symbolic variable. Does that make sense?
Also, don't worry too much about the 'todo' in doc/en/constructions/calculus.rst
. That is truly ancient, from the days where legendary heroes of yore managed to bring calculus functionality, despite awkward syntax, into an algebraic geometry program... meaning eventually that should be just rewritten to use "native syntax" or just folded into the calculus documentation.
comment:36 Changed 5 years ago by
 Commit changed from f70208835cb64305fc90524a9b67c5a86aed0c1e to 2d31fb12b7a2932624a4ba2b310582d190d70824
Branch pushed to git repo; I updated commit sha1. New commits:
2d31fb1  8734: doc cosmetics

comment:37 followup: ↓ 38 Changed 5 years ago by
Okay, here are a few questions. I am pretty sure the answers are very straightforward, but I want to make sure it's clear  in case we might want to add a doctest, for instance.
 Commit be9367 (where you add a try/except clause in
_create
)  what sort of situation is that catching? (Also, did you change all of the doctests with assumptions, or leave a few just so people see what the full form looks like?)  Commit ced268 (where you generalized the missing assumptions)  what situation is that additionally catching? Was that a case of Maxima asking questions which we didn't catch (and hence doctest) before?
 I assume you are more than happy with Jason's original patch doing the basic functionality, right?
The only part I haven't had a chance to really dig into is the differential equations changes (Commit a74ec0). I assume there isn't an obvious more modular way to deal with those :( but I don't have time currently to go through this in detail. My main issue (not really a concern, just my lack of being convinced yet) is just seeing that we really aren't overreplacing or underreplacing in that.
So these are pretty minor things and hopefully I'll be able to find some time for the DE part (or someone else will!) and we'll be on our way! It certainly looks like you were VERY thorough in finding places that might cause trouble. The difficulty is that one might miss some places it is needed because in the absence of adding _SAGE_VAR_
things should still work, so one might not know if we missed one. Thanks!
comment:38 in reply to: ↑ 37 ; followup: ↓ 41 Changed 5 years ago by
Replying to kcrisman:
Okay, here are a few questions. I am pretty sure the answers are very straightforward, but I want to make sure it's clear  in case we might want to add a doctest, for instance.
Sure, as long as you remember "The perfect is the enemy of the good."
 Commit be9367 (where you add a try/except clause in
_create
)  what sort of situation is that catching?
There are several places where maxima.eval() is called and where exceptions are thrown. This one was simply overlooked, so that change fixes an unreported bug.
(Also, did you change all of the doctests with assumptions, or leave a few just so people see what the full form looks like?)
Yes.
 Commit ced268 (where you generalized the missing assumptions)  what situation is that additionally catching? Was that a case of Maxima asking questions which we didn't catch (and hence doctest) before?
No, it reduces code duplication. Recommended reading: https://en.wikipedia.org/wiki/Code_refactoring
 I assume you are more than happy with Jason's original patch doing the basic functionality, right?
Yes and no. A lot was missing.
... So these are pretty minor things and hopefully I'll be able to find some time for the DE part (or someone else will!) and we'll be on our way! It certainly looks like you were VERY thorough in finding places that might cause trouble. The difficulty is that one might miss some places it is needed because in the absence of adding
_SAGE_VAR_
things should still work, so one might not know if we missed one. Thanks!
I would assume this is caught by all those doctests using maxima.
comment:39 Changed 5 years ago by
 Commit changed from 2d31fb12b7a2932624a4ba2b310582d190d70824 to 2c68026320ba3c516cd94fa1721e886ecd732804
Branch pushed to git repo; I updated commit sha1. New commits:
2c68026  8734: adapt doctest to recent change of output

comment:40 Changed 5 years ago by
The output changed recently and I forgot to fix one doctest.
comment:41 in reply to: ↑ 38 ; followup: ↓ 42 Changed 5 years ago by
Okay, here are a few questions. I am pretty sure the answers are very straightforward, but I want to make sure it's clear  in case we might want to add a doctest, for instance.
Sure, as long as you remember "The perfect is the enemy of the good."
Of course!
 Commit be9367 (where you add a try/except clause in
_create
)  what sort of situation is that catching?There are several places where maxima.eval() is called and where exceptions are thrown. This one was simply overlooked, so that change fixes an unreported bug.
Hmm, okay. I don't see how _create
could have asked for an evaluation of this type, but I suppose.
 Commit ced268 (where you generalized the missing assumptions)  what situation is that additionally catching? Was that a case of Maxima asking questions which we didn't catch (and hence doctest) before?
No, it reduces code duplication. Recommended reading: https://en.wikipedia.org/wiki/Code_refactoring
I'm not referring to that; in fact, I fully agreed with that strategy in earlier comments. My question is specifically about jj=2
 since usually jj=3
seems to be the old case. We should test a new branch, which this appears to be (though it may just be something obvious I'm not seeing).
 I assume you are more than happy with Jason's original patch doing the basic functionality, right?
Yes and no. A lot was missing.
I mean in terms of reviewing that for the basic functionality for proper conversion. Naturally you provided a huge amount of missing stuff!
So these are pretty minor things and hopefully I'll be able to find some time for the DE part (or someone else will!) and we'll be on our way! It certainly looks like you were VERY thorough in finding places that might cause trouble. The difficulty is that one might miss some places it is needed because in the absence of adding
_SAGE_VAR_
things should still work, so one might not know if we missed one. Thanks!I would assume this is caught by all those doctests using maxima.
That's my point  they may NOT catch a missing one, since we only remove things via searchandreplacewithemptystring, and everything worked before.
comment:42 in reply to: ↑ 41 ; followup: ↓ 43 Changed 5 years ago by
Replying to kcrisman:
 Commit be9367 (where you add a try/except clause in
_create
)  what sort of situation is that catching?There are several places where maxima.eval() is called and where exceptions are thrown. This one was simply overlooked, so that change fixes an unreported bug.
Hmm, okay. I don't see how
_create
could have asked for an evaluation of this type, but I suppose.
It is really easy to find it out yourself: just change RuntimeError
to something different like NotImplementedError
, doctest, and you'll get:
sage t src/sage/tests/french_book/recequadiff.py ********************************************************************** File "src/sage/tests/french_book/recequadiff.py", line 247, in sage.tests.french_book.recequadiff Failed example: desolve(eq1,[f,x]) Expected: Traceback (most recent call last): ... TypeError: Computation failed ... Is k positive, negative or zero? Got: <BLANKLINE> Traceback (most recent call last): File "/home/ralf/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 480, in _run self.execute(example, compiled, test.globs) File "/home/ralf/sage/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 839, in execute exec compiled in globs File "<doctest sage.tests.french_book.recequadiff[64]>", line 1, in <module> desolve(eq1,[f,x]) File "/home/ralf/sage/local/lib/python2.7/sitepackages/sage/calculus/desolvers.py", line 436, in desolve soln = P(cmd) File "/home/ralf/sage/local/lib/python2.7/sitepackages/sage/interfaces/interface.py", line 199, in __call__ return cls(self, x, name=name) File "/home/ralf/sage/local/lib/python2.7/sitepackages/sage/interfaces/interface.py", line 626, in __init__ raise TypeError(x) TypeError: ECL says: Maxima asks: Is _SAGE_VAR_k positive, negative or zero?
 Commit ced268 (where you generalized the missing assumptions)  what situation is that additionally catching? Was that a case of Maxima asking questions which we didn't catch (and hence doctest) before?
No, it reduces code duplication. Recommended reading: https://en.wikipedia.org/wiki/Code_refactoring
I'm not referring to that; in fact, I fully agreed with that strategy in earlier comments. My question is specifically about
jj=2
 since usuallyjj=3
seems to be the old case. We should test a new branch, which this appears to be (though it may just be something obvious I'm not seeing).
There were cases where there were two spaces in the output after 'Is '.
 I assume you are more than happy with Jason's original patch doing the basic functionality, right?
Yes and no. A lot was missing.
I mean in terms of reviewing that for the basic functionality for proper conversion. Naturally you provided a huge amount of missing stuff!
I think his ansatz was what I would have done, too.
So these are pretty minor things and hopefully I'll be able to find some time for the DE part (or someone else will!) and we'll be on our way! It certainly looks like you were VERY thorough in finding places that might cause trouble. The difficulty is that one might miss some places it is needed because in the absence of adding
_SAGE_VAR_
things should still work, so one might not know if we missed one. Thanks!I would assume this is caught by all those doctests using maxima.
That's my point  they may NOT catch a missing one, since we only remove things via searchandreplacewithemptystring, and everything worked before.
Can you please clarify: "one might miss some places it is needed"what's the it here?
comment:43 in reply to: ↑ 42 Changed 5 years ago by
There were cases where there were two spaces in the output after 'Is '.
Annoying.
That's my point  they may NOT catch a missing one, since we only remove things via searchandreplacewithemptystring, and everything worked before.
Can you please clarify: "one might miss some places it is needed"what's the it here?
What I mean is that if one had missed a place where we should have added SAGE_VAR
but didn't, we might not know because the result would not need SAGE_VAR
stripped from it, so it would perhaps still work. But I'm not overly concerned about this ... because it would still work.
Anyway, if someone reviews the diffeq part before me (I won't have time before next week), that is all I think still needs review. This will be great to finally have in!
comment:44 followup: ↓ 45 Changed 5 years ago by
For the
try: .... except ... as error: ... raise error
in maxima_lib.py (and possibly elsewhere):
It's *much* better to reraise an error with a bare raise
rather than raise error
, since the bare raise will leave the original traceback intact, whereas the raise error
will create a new traceback, obscuring the actual source of the error.
comment:45 in reply to: ↑ 44 ; followup: ↓ 47 Changed 5 years ago by
Replying to nbruin:
For the
try: .... except ... as error: ... raise errorin maxima_lib.py (and possibly elsewhere): It's *much* better to reraise an error with a bare
raise
rather thanraise error
, since the bare raise will leave the original traceback intact, whereas theraise error
will create a new traceback, obscuring the actual source of the error.
In this case, yes. Else, are you requesting to rethrow the RuntimeError
from Maxima in all cases as RuntimeError
? I'm asking because it appears that preserving the stacktrace and throwing the more specific ValueError
appears possible:
http://www.gossamerthreads.com/lists/python/python/947257
In Python 3 you could chain the exceptions with: except Exception as e: raise CustomException() from e There is no such syntax in Python 2, but you could manually store and retrieve the __cause__ and __traceback__ attributes similarly to the way Python 3 does it. See PEP 3134 for full details.
http://legacy.python.org/dev/peps/pep3134/
Sometimes it can be useful for an exception handler to intentionally reraise an exception, either to provide extra information or to translate an exception to another type. ...
comment:46 Changed 5 years ago by
 Commit changed from 2c68026320ba3c516cd94fa1721e886ecd732804 to 793bb058a06b3dfb31c6029f6c30960d1dee8cc7
Branch pushed to git repo; I updated commit sha1. New commits:
793bb05  8734: reraise in "other" cases with bare "raise"

comment:47 in reply to: ↑ 45 ; followup: ↓ 48 Changed 5 years ago by
Replying to rws:
In this case, yes. Else, are you requesting to rethrow the
RuntimeError
from Maxima in all cases asRuntimeError
? I'm asking because it appears that preserving the stacktrace and throwing the more specificValueError
appears possible:
Good to know! I don't have a strong opinion what type of error to return. However, without further inspection, I don't think you would know if the error is more appropriately a ValueError
. You could get RuntimeError("ECL says: I'm tired")
for as far as you know at this point. So my guess is that it's not worth trying to do something with the error type.
comment:48 in reply to: ↑ 47 ; followup: ↓ 49 Changed 5 years ago by
 Dependencies set to #15530
All the following is modulo my Python newbieness.
What I wrote about legal tricks to preserve the traceback would work IF Sage was purely Python2 or Python3. With the momentary transition to Py3, the form raise Error, message, traceback
of the raise command is considered a bug. OTOH there is no way to supply two arguments in parentheses. Finally, the form raise Error from previous_error
is only possible with Py3.
Secondly, to ask interactively for a missing assumption a message has to be given in the terminal different from the error message tied to the original RuntimeError
. So, simply reraising is not an option if you do not like RuntimeError: ECL says: Maxima asks: Is _SAGE_VAR_a an integer?
.
So, in my opinion, unless someone comes up with a better solution, this ticket will have to wait until Sage is Py3.
comment:49 in reply to: ↑ 48 ; followup: ↓ 50 Changed 5 years ago by
Replying to rws:
Secondly, to ask interactively for a missing assumption a message has to be given in the terminal different from the error message tied to the original
RuntimeError
. So, simply reraising is not an option if you do not likeRuntimeError: ECL says: Maxima asks: Is _SAGE_VAR_a an integer?
.
In that case we know exactly what happened and the original traceback doesn't have to be kept. Raising a a fresh exception with a fresh traceback should be fine. It's when you find that the RuntimeError
you've just caught is *not* the one you expected that the original traceback is valuable. And in that case you probably don't want to mess with the exception object itself either, so a bare raise
should do the trick. The scenario of changing the error object but keeping the original traceback should be quite rare.
So, in my opinion, unless someone comes up with a better solution, this ticket will have to wait until Sage is Py3.
which may be a very long time. It nice to do things in a Py2/Py3 compatible way if possible (which can be done here, I think), but if not we'll just have to fix it if/when sage transitions from Py2 to Py3.
comment:50 in reply to: ↑ 49 Changed 5 years ago by
So, in my opinion, unless someone comes up with a better solution, this ticket will have to wait until Sage is Py3.
which may be a very long time.
Indeed.
It nice to do things in a Py2/Py3 compatible way if possible (which can be done here, I think), but if not we'll just have to fix it if/when sage transitions from Py2 to Py3.
I'm not quite clear on why this is the case. I thought you said above to not let the perfect be the enemy of the good ;) It seems like just raising the original exception is a good compromise here; at the very least it should give some information, right?
comment:51 Changed 5 years ago by
I agree I was the perfectionist this time so, with the bare raise patch already uploaded, it remains for me to thank you and ask for positive review.
comment:52 Changed 5 years ago by
Again, for me, all is positive review except the diffeq commit, and that only because I haven't had time to look carefully at it, not because I suspect any problems  it looks quite thorough.
comment:53 Changed 5 years ago by
 Dependencies #15530 deleted
comment:54 followups: ↓ 55 ↓ 56 ↓ 58 Changed 5 years ago by
Okay, I have a few questions about the ode changes. I assume the answer to all of them is "it would have been even messier any other way", but I just wanted to check.
 In some of the sanitizing functions, you replace things like
'_SAGE_VAR_f
withf
, but in others you only replace the independent variable that way. Is that because of specific examples that didn't work, or was the context different, or... ?  I'm wondering whether the Sage translation would have just taken care of this in
soln.sage()
, but I guess it didn't. Was there any possible change to the translation that could have done this, rather than getting into the ode wrapper code directly (which makes it harder to read)? For instance, indesolve_laplace
we convert thede
to Maxima (presumably addingSAGE_VAR
, add anotherSAGE_VAR
fromf(x)
tof(_SAGE_VAR_x)
(I think), and then proceed to remove only theSAGE_VAR
from the dependent variable. So... that part isn't taken care of by the translation, but the independent variable still somehow is translated back correctly, but not forward withinde0=de._maxima_()
? Yet in therk4
types this isn't a problem, apparently.  We should probably just remove
desolve_system_strings
, see #8132 where it was first said to be obsolete, and it hasn't been in the global namespace since before 2010. That is pretty much equivalent to a deprecation to me. However, we should keep any nonoverlapping examples  so maybe removal should be another ticket...
But it seems good, assuming I didn't miss any tests that fail...
comment:55 in reply to: ↑ 54 Changed 5 years ago by
But it seems good, assuming I didn't miss any tests that fail...
I didn't. So as long as you give the answers I expect, we are all set here.
comment:56 in reply to: ↑ 54 Changed 5 years ago by
Working from the bottom up ...
Replying to kcrisman:
 We should probably just remove
desolve_system_strings
, see #8132 where it was first said to be obsolete, and it hasn't been in the global namespace since before 2010. That is pretty much equivalent to a deprecation to me. However, we should keep any nonoverlapping examples  so maybe removal should be another ticket...
This is now #16568.
comment:57 Changed 5 years ago by
 Commit changed from 793bb058a06b3dfb31c6029f6c30960d1dee8cc7 to a195985c0617bf14a5d644f5c065bea896340ce4
Branch pushed to git repo; I updated commit sha1. New commits:
a195985  8734: remove superfluous code

comment:58 in reply to: ↑ 54 ; followup: ↓ 59 Changed 5 years ago by
Replying to kcrisman:
 In some of the sanitizing functions, you replace things like
'_SAGE_VAR_f
withf
, but in others you only replace the independent variable that way. Is that because of specific examples that didn't work, or was the context different, or... ?
It certainly was then because of specific examples. The removal of the code I think you mean makes no difference now, however.
 I'm wondering whether the Sage translation would have just taken care of this in
soln.sage()
, but I guess it didn't. Was there any possible change to the translation that could have done this, rather than getting into the ode wrapper code directly (which makes it harder to read)? For instance, indesolve_laplace
we convert thede
to Maxima (presumably addingSAGE_VAR
, add anotherSAGE_VAR
fromf(x)
tof(_SAGE_VAR_x)
(I think), and then proceed to remove only theSAGE_VAR
from the dependent variable. So... that part isn't taken care of by the translation, but the independent variable still somehow is translated back correctly, but not forward withinde0=de._maxima_()
? Yet in therk4
types this isn't a problem, apparently.
With the above removal of superfluous code I don't see any different behaviour in the three functions laplace/system/rk4
. There is always the marking of the dependent var to prepare for the call to solve. You cannot remove this because maxima(cmd)
does no translation, it's the lowlevel call. The translation has to be done here, as far as I understand.
comment:59 in reply to: ↑ 58 Changed 5 years ago by
With the above removal of superfluous code I don't see any different behaviour in the three functions
laplace/system/rk4
. There is always the marking of the dependent var to prepare for the call to solve. You cannot remove this becausemaxima(cmd)
does no translation, it's the lowlevel call. The translation has to be done here, as far as I understand.
Again, I'm just confused because in laplace/rk4
the dependent variable gets _SAGE_VAR_
but in desolve
it doesn't. Maybe this isn't important, though. And is the last change in desolve
ok because putting things into Maxima takes care of the _SAGE_VAR_
already?
comment:60 Changed 5 years ago by
Passes all tests.
comment:61 Changed 5 years ago by
After studying this in detail, the reason why the last change makes no difference is the following: in desolve
and desolve_laplace
the translation to Maxima is applied several times using maxima()
and P()
(which is the parent of the first translation result, i.e. of a Maxima expression). P()
is also applied to dvar.operator()
resulting in dvar_str
which already has _SAGE_VAR_
and is template for sanitization. Appending _SAGE_VAR_
to dvar_str
and replacing it thus was useless.
In desolve_rk4
the cmd
is constructed from two parts: the de0
that gets translated via ._maxima()_
and the construction via string concatenation. So, there is no difference between desolve
and desolve_rk4
because desolve
gets _SAGE_VAR_
everywhere and desolve_rk4
too.
comment:62 Changed 5 years ago by
 Status changed from needs_review to positive_review
Let's make it so, then. I was kind of suspecting there were extra _SAGE_VAR_
s.
comment:63 Changed 5 years ago by
 Status changed from positive_review to needs_work
conflicts with #6882
comment:64 Changed 5 years ago by
 Commit changed from a195985c0617bf14a5d644f5c065bea896340ce4 to 7a6696b714e638461cf4f77adbb65ecf413a2e4e
Branch pushed to git repo; I updated commit sha1. New commits:
65107d4  Merge branch 'develop' into t/8734/ticket/87341

4e07383  6882: add rules for e, i, I

e5a5343  6882: do word search/replace for symtable keys

4c1b0eb  6882: correction to previous commit

518de3e  6882: add symable rules for e,i,I; fix maxima_var; add doctests

7a6696b  Merge branch 'u/rws/bugs_in_conversion_of_variable_names_from_maxima_to_sage' of trac.sagemath.org:sage into t/8734/ticket/87341

comment:65 followup: ↓ 67 Changed 5 years ago by
 Dependencies set to #6882
 Status changed from needs_work to positive_review
comment:66 Changed 5 years ago by
 Reviewers changed from Volker Braun, Paul Zimmerman, KarlDieter Crisman to Volker Braun, Paul Zimmermann, KarlDieter Crisman
comment:67 in reply to: ↑ 65 Changed 5 years ago by
I would be happy if you could have a look at #2516 too.
Sorry, that one I definitely like the idea of and have reviewed many similar ones in the past, but just don't have the time currently to review much new functionality carefully. :(
comment:68 Changed 5 years ago by
 Branch changed from u/rws/ticket/87341 to 7a6696b714e638461cf4f77adbb65ecf413a2e4e
 Resolution set to fixed
 Status changed from positive_review to closed
I haven't run doctests on everything, but this patch is a start, if one of you wants to take it from here.