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: sage-6.3
Component: interfaces Keywords:
Cc: was, acleone, kcrisman, robert.marik, burcin, saliola Merged in:
Authors: Jason Grout, Ralf Stephan Reviewers: Volker Braun, Paul Zimmermann, Karl-Dieter 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)

trac-8734-maxima-vars.patch (4.7 KB) - added by jason 9 years ago.

Download all attachments as: .zip

Change History (69)

Changed 9 years ago by jason

comment:1 Changed 9 years ago by jason

  • Cc was acleone added
  • Status changed from new to needs_work

I haven't run doctests on everything, but this patch is a start, if one of you wants to take it from here.

comment:2 Changed 9 years ago by kcrisman

  • Cc kcrisman added

I think there might also be another ticket about this somewhere...

comment:3 Changed 9 years ago by robert.marik

Hi, it is also #8634 and there is a dicussion at sage-devel

comment:4 Changed 9 years ago by robert.marik

  • Cc robert.marik added

comment:5 follow-up: Changed 9 years ago by jason

  • Owner changed from was to jason

This should fix #6882

comment:6 in reply to: ↑ 5 Changed 9 years ago by jason

Replying to jason:

This should fix #6882

Well, or at least *help* with the solution, anyway.

comment:7 Changed 9 years ago by burcin

  • Cc burcin added

comment:8 Changed 9 years ago by saliola

  • Cc saliola added

comment:9 Changed 9 years ago by mpatel

This ticket may also fix #9538.

comment:10 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:11 Changed 6 years ago by zimmerma

  • Work issues set to rebase

patch should be rebased, it fails to apply to Sage 5.11:

sage: hg_sage.import_patch("/tmp/trac-8734-maxima-vars.patch")
cd "/home/zimmerma/Downloads/sage-5.11/devel/sage" && sage --hg import   "/tmp/trac-8734-maxima-vars.patch"
applying /tmp/trac-8734-maxima-vars.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 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:13 Changed 6 years ago by rws

  • 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 6 years ago by rws

  • 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:

87d944fTrac #8734: Make sage variables unique in maxima.

comment:15 Changed 6 years ago by git

  • Commit changed from 87d944ffbe59afe29de2faea2b569f7eaa904295 to 0ee7f20ff6a003d52224c84510e667483c6ddc2b

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

5b76872Merge branch 'develop' into needswork/8734
ca6a221factor out missing assumption error handling; filter _SAGE_VAR_
3b7e916fix typo leading to error
0ee7f20two further maxima calls adapted; two more doctests fixed

comment:16 Changed 6 years ago by rws

  • Authors changed from Jason Grout to Jason Grout, Ralf Stephan
  • 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 6 years ago by kcrisman

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 non-existent (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 6 years ago by git

  • Commit changed from 0ee7f20ff6a003d52224c84510e667483c6ddc2b to a5f27eed1dfb83e1c979ecb23ad2854ccd00400b

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

b1cd2bffix and generalize missing_assumption(); resp. doctest adaptions
b4df332Merge branch 'develop' into needswork/8734
a5f27eeadd even more cases to add _SAGE_VAR_; fix doctests

comment:19 Changed 6 years ago by rws

  • 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 vbraun

Merge conflict, please merge in the latest beta

comment:21 Changed 5 years ago by rws

  • Branch changed from u/rws/ticket/8734 to u/rws/ticket/8734-1
  • Commit changed from a5f27eed1dfb83e1c979ecb23ad2854ccd00400b to f00cd8d5dc695dee49f59164251772db46bf48c3
  • Work issues fix doctest problems deleted

New commits:

3d90c49Trac #8734: Make sage variables unique in maxima.
a6cbf80factor out missing assumption error handling; filter _SAGE_VAR_
3f24835fix typo leading to error
f36c6b5two further maxima calls adapted; two more doctests fixed
ced268dfix and generalize missing_assumption(); resp. doctest adaptions
a74ec00add even more cases to add _SAGE_VAR_; fix doctests
f00cd8dMerge branch 'rev/8734' into tmp

comment:22 Changed 5 years ago by vbraun

  • Reviewers set to Volker Braun

comment:23 Changed 5 years ago by vbraun

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 vbraun

  • Status changed from needs_review to needs_work

comment:25 Changed 5 years ago by git

  • Commit changed from f00cd8d5dc695dee49f59164251772db46bf48c3 to be9367f966fc76ba6028b57ab4317530cfd1205e

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

4a570d88734: more additions of _SAGE_VAR_ in doctests
be9367f8734: one more maxima call adapted; more doctests fixed

comment:26 follow-up: Changed 5 years ago by rws

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 rws

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 git

  • Commit changed from be9367f966fc76ba6028b57ab4317530cfd1205e to 06b952805ab10b7f30b4c06c1fcd487cbf1b7dce

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

1e9b54b8734: more doctests fixed
06b95288734: fix doctests

comment:29 Changed 5 years ago by rws

  • Status changed from needs_work to needs_review

comment:30 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:31 Changed 5 years ago by git

  • Commit changed from 06b952805ab10b7f30b4c06c1fcd487cbf1b7dce to 6fffc21fbfe9a72f40f51ba22efec6dd9ec28ac5

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

6fffc21Merge branch 'develop' into t/8734/ticket/8734-1

comment:32 Changed 5 years ago by git

  • Commit changed from 6fffc21fbfe9a72f40f51ba22efec6dd9ec28ac5 to 3d74cad55ddc6d1a0c64356a59c88cdac1a6f2d9

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

3d74cad8734: rename private function, doctest it

comment:33 Changed 5 years ago by git

  • Commit changed from 3d74cad55ddc6d1a0c64356a59c88cdac1a6f2d9 to cec9681b1ebb379ed6eccec389981512acd527cb

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

3180e7bMerge branch 'develop' into t/8734/ticket/8734-1
cec96818734: merge conflicts

comment:34 Changed 5 years ago by git

  • Commit changed from cec9681b1ebb379ed6eccec389981512acd527cb to f70208835cb64305fc90524a9b67c5a86aed0c1e

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

f702088take back unrelated change

comment:35 Changed 5 years ago by kcrisman

  • Reviewers changed from Volker Braun to Volker Braun, Paul Zimmerman, Karl-Dieter 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 git

  • Commit changed from f70208835cb64305fc90524a9b67c5a86aed0c1e to 2d31fb12b7a2932624a4ba2b310582d190d70824

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

2d31fb18734: doc cosmetics

comment:37 follow-up: Changed 5 years ago by 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.

  • 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 over-replacing or under-replacing 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 ; follow-up: Changed 5 years ago by rws

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 git

  • Commit changed from 2d31fb12b7a2932624a4ba2b310582d190d70824 to 2c68026320ba3c516cd94fa1721e886ecd732804

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

2c680268734: adapt doctest to recent change of output

comment:40 Changed 5 years ago by rws

The output changed recently and I forgot to fix one doctest.

comment:41 in reply to: ↑ 38 ; follow-up: Changed 5 years ago by 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."

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 search-and-replace-with-empty-string, and everything worked before.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 5 years ago by rws

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/site-packages/sage/doctest/forker.py", line 480, in _run
        self.execute(example, compiled, test.globs)
      File "/home/ralf/sage/local/lib/python2.7/site-packages/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/site-packages/sage/calculus/desolvers.py", line 436, in desolve
        soln = P(cmd)
      File "/home/ralf/sage/local/lib/python2.7/site-packages/sage/interfaces/interface.py", line 199, in __call__
        return cls(self, x, name=name)
      File "/home/ralf/sage/local/lib/python2.7/site-packages/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 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).

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 search-and-replace-with-empty-string, 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 kcrisman

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 search-and-replace-with-empty-string, 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 follow-up: Changed 5 years ago by nbruin

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 ; follow-up: Changed 5 years ago by rws

Replying to nbruin:

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.

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.gossamer-threads.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/pep-3134/

Sometimes it can be useful for an exception handler to intentionally
    re-raise an exception, either to provide extra information or to
    translate an exception to another type. ...

comment:46 Changed 5 years ago by git

  • Commit changed from 2c68026320ba3c516cd94fa1721e886ecd732804 to 793bb058a06b3dfb31c6029f6c30960d1dee8cc7

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

793bb058734: reraise in "other" cases with bare "raise"

comment:47 in reply to: ↑ 45 ; follow-up: Changed 5 years ago by nbruin

Replying to rws:

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:

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 ; follow-up: Changed 5 years ago by rws

  • 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 Python-2 or Python-3. With the momentary transition to Py-3, 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 Py-3.

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 re-raising 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 Py-3.

comment:49 in reply to: ↑ 48 ; follow-up: Changed 5 years ago by nbruin

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 re-raising is not an option if you do not like RuntimeError: 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 Py-3.

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 kcrisman

So, in my opinion, unless someone comes up with a better solution, this ticket will have to wait until Sage is Py-3.

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 rws

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 kcrisman

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 rws

  • Dependencies #15530 deleted

comment:54 follow-ups: Changed 5 years ago by kcrisman

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 with f, 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, in desolve_laplace we convert the de to Maxima (presumably adding SAGE_VAR, add another SAGE_VAR from f(x) to f(_SAGE_VAR_x) (I think), and then proceed to remove only the SAGE_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 within de0=de._maxima_()? Yet in the rk4 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 non-overlapping 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 kcrisman

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 rws

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 non-overlapping examples - so maybe removal should be another ticket...

This is now #16568.

comment:57 Changed 5 years ago by git

  • Commit changed from 793bb058a06b3dfb31c6029f6c30960d1dee8cc7 to a195985c0617bf14a5d644f5c065bea896340ce4

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

a1959858734: remove superfluous code

comment:58 in reply to: ↑ 54 ; follow-up: Changed 5 years ago by rws

Replying to kcrisman:

  • In some of the sanitizing functions, you replace things like '_SAGE_VAR_f with f, 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, in desolve_laplace we convert the de to Maxima (presumably adding SAGE_VAR, add another SAGE_VAR from f(x) to f(_SAGE_VAR_x) (I think), and then proceed to remove only the SAGE_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 within de0=de._maxima_()? Yet in the rk4 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 low-level call. The translation has to be done here, as far as I understand.

comment:59 in reply to: ↑ 58 Changed 5 years ago by kcrisman

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 low-level 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 kcrisman

Passes all tests.

comment:61 Changed 5 years ago by rws

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.

Last edited 5 years ago by rws (previous) (diff)

comment:62 Changed 5 years ago by kcrisman

  • 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 vbraun

  • Status changed from positive_review to needs_work

conflicts with #6882

comment:64 Changed 5 years ago by git

  • Commit changed from a195985c0617bf14a5d644f5c065bea896340ce4 to 7a6696b714e638461cf4f77adbb65ecf413a2e4e

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

65107d4Merge branch 'develop' into t/8734/ticket/8734-1
4e073836882: add rules for e, i, I
e5a53436882: do word search/replace for symtable keys
4c1b0eb6882: correction to previous commit
518de3e6882: add symable rules for e,i,I; fix maxima_var; add doctests
7a6696bMerge branch 'u/rws/bugs_in_conversion_of_variable_names_from_maxima_to_sage' of trac.sagemath.org:sage into t/8734/ticket/8734-1

comment:65 follow-up: Changed 5 years ago by rws

  • Dependencies set to #6882
  • Status changed from needs_work to positive_review

Since the reviewed #6882 is orthogonal I set positive again. Thanks Karl-Dieter for this review! I would be happy if you could have a look at #2516 too.

comment:66 Changed 5 years ago by zimmerma

  • Reviewers changed from Volker Braun, Paul Zimmerman, Karl-Dieter Crisman to Volker Braun, Paul Zimmermann, Karl-Dieter Crisman

comment:67 in reply to: ↑ 65 Changed 5 years ago by kcrisman

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 vbraun

  • Branch changed from u/rws/ticket/8734-1 to 7a6696b714e638461cf4f77adbb65ecf413a2e4e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.