#16007 closed defect (fixed)
give solution constants of ODEs unique names
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage6.3 
Component:  calculus  Keywords:  
Cc:  kcrisman  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Nils Bruin, KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  a8df107 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  #8734  Stopgaps: 
Description (last modified by )
(this was reported as part of #6882)
... solving of ode y'=c*x is not correct, the free variable is messed up with a parameter, see sagedevel  thanks for Yuri Karadzhov
sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string sage: symbolic_expression_from_maxima_string('%c') c sage: c=var('c'); y=function('y',x); eq=diff(y,x)==c*x; eq D[0](y)(x) == c*x sage: desolve(eq,y,ivar=x) 1/2*c*x^2 + c
the answer should be something like 1/2*c*x^{2} + c1
The fix depends on closing #8734. The c
variable that comes from Sage can then be easily recognized, and the constant c
should then be renamed to c1
or _C1
.
Change History (52)
comment:1 Changed 8 years ago by
 Cc kcrisman added
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
 Branch set to u/rws/ticket/16007
 Created changed from 03/25/14 10:09:58 to 03/25/14 10:09:58
 Modified changed from 03/26/14 06:23:25 to 03/26/14 06:23:25
comment:4 Changed 8 years ago by
 Commit set to ee630f53b20aab08d094f0f9914875f30efd5e66
 Dependencies #8734 deleted
 Status changed from new to needs_review
Indeed, many thanks. With this easy patch the output is now
sage: sage: x=function('x',t) sage: sage: var('c') c sage: sage: desolve(diff(x,t)+2*x==t^22*t+c,x,ivar=t).expand() 1/2*t^2 + _C*e^(2*t) + 1/2*c  3/2*t + 3/4 sage: desolve(diff(f(x),x,x)f(x),f(x)) _K2*e^(x) + _K1*e^x
Any more variables?
New commits:
ee630f5  add %c,%k1,%k2 to recognized maxima objects

comment:5 Changed 8 years ago by
seems to be a duplicate of #9421.
Paul
comment:6 Changed 8 years ago by
Yes. So, are you okay with this solution applied to #9421, instead of the warning? If so, then I will make a reviewer's patch there and invalid this ticket.
comment:7 Changed 8 years ago by
yes I agree with this solution, but I won't have time quite soon to check the doctests still pass.
Paul
comment:8 Changed 8 years ago by
It might be worth doing a few things. First, you must have documentation testing this fix  especially since people will be confused by underscore variables without some documentation! Also, how many different things can show up? Just c
, k1
, k2
, or also c1
et al, or k3
, ... ? It would be worth asking on the Maxima email list about what outputs can possibly happen (assuming it's working  I was at some point subscribed to the "new" version but now am apparently not again).
But I agree it's very unlikely for anyone to have an underscore variable, so this could be a good compromise... there remains the issue of substituting in for nonSageified variables.
comment:9 Changed 8 years ago by
 Status changed from needs_review to needs_work
comment:10 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:11 Changed 8 years ago by
OK. Then what remains is a warning after the second line. Could you please supply a good wording?
comment:12 followup: ↓ 13 Changed 8 years ago by
 Status changed from needs_work to needs_review
Maximadiscuss list mail:
On 05062014 16:36, Ralf Stephan wrote: >Sage uses Maxima to solve ODEs. To do that bug free we would >need to know which constant vars apart from %c, %k1, %k2 are >returned by Maxima. Can you please help? looking at the source code of the programs in share/diffequations, it seems to me that those 3 are the only ones. I don't know if you have other programs in mind, apart from those in share/diffequations. Regards, Jaime
I take it then the ticket is complete for review.
comment:13 in reply to: ↑ 12 Changed 8 years ago by
Thanks for doing that due diligence. Yes, I believe these only arise from a contrib package in Maxima.
I take it then the ticket is complete for review.
Well... see in comment:8
First, you must have documentation testing this fix  especially since people will be confused by underscore variables without some documentation!
It's really unfortunate with the underscores. But I hate to think of checking whether C
, C1
, C2
etc. were defined and then using whatever the next one was. Anyway, unless Nils (who knows this stuff better than me) objects to your solution (quoting comment:9:ticket:9421)
(with the righter solution being: making sage's "forget the %" more intelligent, so that collisions can be avoided)
then as I said, this is definitely much better than leaving it 'as is'.
comment:14 Changed 8 years ago by
 Commit changed from ee630f53b20aab08d094f0f9914875f30efd5e66 to 630ea9299705f8aa029e6ef38d8055b7c5794f52
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
1e9b54b  8734: more doctests fixed

06b9528  8734: fix doctests

6fffc21  Merge branch 'develop' into t/8734/ticket/87341

3d74cad  8734: rename private function, doctest it

3180e7b  Merge branch 'develop' into t/8734/ticket/87341

cec9681  8734: merge conflicts

f702088  take back unrelated change

2d31fb1  8734: doc cosmetics

100c8eb  Merge branch 't/8734/ticket/87341' into t/16007/ticket/16007

630ea92  16007: fix doctests; add documentation

comment:15 followup: ↓ 16 Changed 8 years ago by
Merged in #8734. Thanks for the doctest reminder.
comment:16 in reply to: ↑ 15 Changed 8 years ago by
Merged in #8734. Thanks for the doctest reminder.
No problem. Do you think it's useful to add an example where there might have otherwise been a collision of names, or is that superfluous?
Also, why is the doctest
sefms('%k1*%x + %k2*%y + %c')
and not
sefms('%k1*x + %k2*y + %c')
I'm not sure what the %
is doing on the variables, though I don't doubt Sage would strip those percents off, if I recall that part of the code correctly (it's been a while, though).
comment:17 Changed 8 years ago by
 Commit changed from 630ea9299705f8aa029e6ef38d8055b7c5794f52 to 92a3fa3739fb069939c741cb310a0ba3cddad9fe
Branch pushed to git repo; I updated commit sha1. New commits:
92a3fa3  16007: make doctest test intended case

comment:18 Changed 8 years ago by
OK. Though not wrong, it wasn't what I intended, so it's changed now.
comment:19 followup: ↓ 20 Changed 8 years ago by
I realized another question. I can't build a branch right now to check this, but I believe that in the past the c
and friends were not only confusing, they didn't actually correspond to any Sage variable. So now we have it that there is unlikely collision, but even then we still have _C
and friends not corresponding to any Sage variable  they haven't been injected into any namespace. (And I don't think we want to do it at startup on the off chance someone needs them.) We may want to do a seekandfind for them but doing it every time could be annoying/add a little time. Anyway, at the very least that should probably be (finally!) documented if we're going to fix this.
Sorry for all the minor things occurring to me.
comment:20 in reply to: ↑ 19 ; followup: ↓ 21 Changed 8 years ago by
Replying to kcrisman:
I realized another question. I can't build a branch right now to check this, but I believe that in the past the
c
and friends were not only confusing, they didn't actually correspond to any Sage variable. So now we have it that there is unlikely collision, but even then we still have_C
and friends not corresponding to any Sage variable
Uh ... as soon as these make it back in sage there are symbolic variable objects that represent them. They are fullblown pynac objects right then.
 they haven't been injected into any namespace.
That has never been a requirement for objects to "exist" in sage. I don't think this is an issue at all. The objects can be accessed via SR("_C")
as usual, and in fact if the user does _C=var("_C")
or toplevel var("_C")
he/she can have the object bound. We should absolutely not mess with namespaces during runtime. We might prebind _C
etc. if someone thinks it's too difficult for users otherwise.
comment:21 in reply to: ↑ 20 ; followup: ↓ 23 Changed 8 years ago by
Replying to nbruin:
We might prebind
_C
etc. if someone thinks it's too difficult for users otherwise.
I would like to. How would I do it?
comment:22 Changed 7 years ago by
You can already have them predefined by putting in $HOME/.sage/init.sage
something like:
_C = SR.var("_C") _K1 = SR.var("_K1") _K2 = SR.var("_K2")
Various notebook/mathcloud solutions probably expose this mechanism or something equivalent somehow.
The error people get when they do try to access these objects via the correspondingly named variables is pretty clear and they'll probably do the right thing next (they've already been able to define a differential equation in sage, so they've had to jump some hurdles already). I'd propose holding off on filling global namespace now and only consider that if it turns out to be a common complaint. The "c,k1,k2" we had before suffered the same problem and didn't generate complaints either.
comment:23 in reply to: ↑ 21 Changed 7 years ago by
We might prebind
_C
etc. if someone thinks it's too difficult for users otherwise.I would like to. How would I do it?
I think my point is that presumably if one then retyped the answer, or tried to use it somehow, one would get an error about an undefined variable.
sage: var('d') d sage: y=function('y',x); eq=diff(y,x)==d*x; eq D[0](y)(x) == d*x sage: desolve(eq,y,ivar=x) 1/2*d*x^2 + c sage: c  NameError Traceback (most recent call last) <ipythoninput42cd6ee2c70b0> in <module>() > 1 c NameError: name 'c' is not defined
That said,
sage: D = desolve(eq,y,ivar=x) sage: D.subs(c=3) 1/2*d*x^2 + 3
works. So I would be comfortable with very good documentation or something ahead of time  and Nils' comment indicates that better documentation for how to use these is best.
comment:24 Changed 7 years ago by
 Commit changed from 92a3fa3739fb069939c741cb310a0ba3cddad9fe to 884bc6841811077d55a9048d616cb973e313ef24
Branch pushed to git repo; I updated commit sha1. New commits:
884bc68  16007: give advice how to use constants

comment:25 Changed 7 years ago by
Please don't forget to add your names.
comment:26 Changed 7 years ago by
 Reviewers set to Nils Bruin, KarlDieter Crisman
comment:27 followup: ↓ 28 Changed 7 years ago by
Not positive? So what's missing?
comment:28 in reply to: ↑ 27 Changed 7 years ago by
Not positive? So what's missing?
Still working on it! I wanted to look more closely at #8734 first. It does look okay, but I just wanted to think a little more about any unintended interactions.
comment:29 Changed 7 years ago by
 Status changed from needs_review to needs_work
Is the line
sage: from sage.calculus.calculus import symbolic_expression_from_maxima_string as sefms
necessary in the doctest? Though it doesn't hurt anything.
sage: sefms('%k1*x + %k2*y + %c') _K1*x + _K2*y + _C sage: _._latex_() '_{K_{1}} x + _{K_{2}} y + _{C}'
I have a feeling this is not what we are looking for, but I don't know how to deal with it best. Presumably 'naked variables/constants' don't need to be subscripted in typesetting.
One idea, which mimics our handling of lambda, would be to make the constants have names k1_
and c_
instead.
sage: var('lambda_') lambda_ sage: latex(_) \lambda sage: var('beta_') beta_ sage: latex(_) \beta sage: var('k1_') k1_ sage: latex(_) \mathit{k1}
Note that this is not what is desired either, since
sage: latex(k1) k_{1} sage: latex(k1_) \mathit{k1} sage: latex(c_) c
I don't know whether this is a good solution, but we do need proper typesetting of these guys.
comment:30 Changed 7 years ago by
Indeed. I would propose to use c_
, k_
, and K_
then, because %k2
rarely appears. Changing the behaviour of latex()
wrt to leading underscore will open the next can of worms, like complaints about typesetting _3F_1
.
comment:31 followup: ↓ 34 Changed 7 years ago by
On the other hand, no. The best solution is in latex_variable_name()
to recognize if a varname matches one entry of the symtable
in calculus.py
.
comment:32 Changed 7 years ago by
 Commit changed from 884bc6841811077d55a9048d616cb973e313ef24 to fe2a0f0c4c536e2ad4f5fb940ce6ee7e3a1f0c22
Branch pushed to git repo; I updated commit sha1. New commits:
fe2a0f0  16007: handle latexification of converted special maxima variable names

comment:33 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:34 in reply to: ↑ 31 Changed 7 years ago by
On the other hand, no. The best solution is in
latex_variable_name()
to recognize if a varname matches one entry of thesymtable
incalculus.py
.
Really? But now the distinction between c
and _c
is lost in typesetting, if I understand this correctly, though I don't know how one would typeset the underscore in any case. I guess there probably isn't any better way, though  or? Ideas?
comment:35 Changed 7 years ago by
Well, now there is a code point where the behaviour can be changed. So, again, is there anything missing?
comment:36 followup: ↓ 38 Changed 7 years ago by
though I don't know how one would typeset the underscore in any case.
Turns out it currently just keeps the underscore.
This change could minutely slow down latex for variables that start with underscores, or if the symtable gets huge... but that is probably silly to worry about for now, neither are likely in the near future. Also may help with #6882. So I'm happy with this commit.
So, again, is there anything missing?
Doctest (presumably just in latex_variable_name
) for the latexing. I realize this seems like a lot of rounds, but I've found the review process very helpful in the past, anyway. (And I'm sorry I haven't time to check out the last commit of #8734 yet.)
comment:37 Changed 7 years ago by
 Commit changed from fe2a0f0c4c536e2ad4f5fb940ce6ee7e3a1f0c22 to 28a08aa0c6a28100d4d2e33643b0bb23b6831bb5
comment:38 in reply to: ↑ 36 Changed 7 years ago by
Replying to kcrisman:
So, again, is there anything missing?
Doctest (presumably just in
latex_variable_name
) for the latexing. I realize this seems like a lot of rounds, but I've found the review process very helpful in the past, anyway. (And I'm sorry I haven't time to check out the last commit of #8734 yet.)
I appreciate your thoroughness, and thanks for the review!
comment:39 Changed 7 years ago by
Okay, if doctests pass (currently attempting to build the branch but it will take a while) then I'm happy with this. Thanks for working on so many symbolics/calculus things recently.
comment:40 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:41 Changed 7 years ago by
 Status changed from positive_review to needs_work
Doctests don't pass (try make ptestlong
)
sage t long src/doc/en/tutorial/tour_algebra.rst ********************************************************************** File "src/doc/en/tutorial/tour_algebra.rst", line 156, in doc.en.tutorial.tour_algebra Failed example: desolve(DE, [x,t]) Expected: (c + e^t)*e^(t) Got: (_C + e^t)*e^(t) **********************************************************************
comment:42 Changed 7 years ago by
Nuts!!! I could have sworn I checked everything... and now I see why long tests on #8734 are taking so long too, I did make testlong
and not make ptestlong
. Very sorry about this.
comment:43 Changed 7 years ago by
 Commit changed from 28a08aa0c6a28100d4d2e33643b0bb23b6831bb5 to 4add98a4e671996e859bb1c008fd61909e982c20
Branch pushed to git repo; I updated commit sha1. New commits:
2c68026  8734: adapt doctest to recent change of output

793bb05  8734: reraise in "other" cases with bare "raise"

b5857b7  Merge branch 'u/rws/ticket/87341' of trac.sagemath.org:sage into t/16007/ticket/16007

4add98a  16007: fix documentation examples

comment:44 Changed 7 years ago by
 Status changed from needs_work to needs_review
Not all of #8734 was merged. Sorry for the inconvenience.
comment:45 Changed 7 years ago by
So has the code from #8734 been reviewed? If not make it a dependency here so I don't accidentally merge it.
comment:46 Changed 7 years ago by
 Dependencies set to #8734
comment:47 Changed 7 years ago by
 Commit changed from 4add98a4e671996e859bb1c008fd61909e982c20 to 67426e10a84f9f73ad8c38d63872b3debb0f794a
comment:48 Changed 7 years ago by
I am not going to check whether this passes tests, but at any rate the changes most recent seem fine, although I don't know why the nondoctest pieces had to have the constant C
instead of c
(but I don't really care about that). So if anyone checks it passes tests we are good here, since #8734 is positive review.
comment:49 Changed 7 years ago by
 Commit changed from 67426e10a84f9f73ad8c38d63872b3debb0f794a to a8df107e76527d83a87456520395ab85dbc44050
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

a8df107  Merge branch 't/8734/ticket/87341' into t/16007/ticket/16007

comment:50 Changed 7 years ago by
Thanks. Pulled in changes from dependencies. I'll run a patchbot on this now.
comment:51 Changed 7 years ago by
 Branch changed from u/rws/ticket/16007 to a8df107e76527d83a87456520395ab85dbc44050
 Resolution set to fixed
 Status changed from needs_review to closed
comment:52 Changed 6 years ago by
 Commit a8df107e76527d83a87456520395ab85dbc44050 deleted
 Description modified (diff)
Actually, these variables are very easy to recognize any way:
As you can see, maxima creates these constants as
%c, %k1, %k2
, so there's no collision there. The collision only happens upon conversion back to sage. So we make the conversion more intelligent there wouldn't be an issue at all. Usingsage.interfaces.maxima_lib.max_to_sr
this would be fairly straightforward.