Opened 5 years ago

Closed 5 years ago

# give solution constants of ODEs unique names

Reported by: Owned by: rws major sage-6.3 calculus kcrisman Ralf Stephan Nils Bruin, Karl-Dieter Crisman N/A a8df107 (Commits) #8734

(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 sage-devel - 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*x2 + 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.

### comment:2 Changed 5 years ago by nbruin

Actually, these variables are very easy to recognize any way:

sage: function('f',x)
f(x)
sage: var('c')
c
sage: W=diff(f(x),x,x)-f(x)+c
sage: V=diff(f(x),x)-f(x)+c
sage: maxima_calculus(V).ode2(f(x),x)
'f(x)=(c*%e^-x+%c)*%e^x
sage: maxima_calculus(W).ode2(f(x),x)
'f(x)=%k1*%e^x+%k2*%e^-x+c


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. Using sage.interfaces.maxima_lib.max_to_sr this would be fairly straightforward.

### comment:3 Changed 5 years ago by rws

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

• Authors set to Ralf Stephan
• 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^2-2*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 5 years ago by zimmerma

seems to be a duplicate of #9421.

Paul

### comment:6 Changed 5 years ago by rws

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 5 years ago by zimmerma

yes I agree with this solution, but I won't have time quite soon to check the doctests still pass.

Paul

### comment:8 Changed 5 years ago by kcrisman

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 non-Sageified variables.

### comment:9 Changed 5 years ago by rws

• Status changed from needs_review to needs_work

### comment:10 Changed 5 years ago by vbraun_spam

• Milestone changed from sage-6.2 to sage-6.3

### comment:11 Changed 5 years ago by rws

Deleted misplaced comment.

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

### comment:12 follow-up: ↓ 13 Changed 5 years ago by rws

• Status changed from needs_work to needs_review

Maxima-discuss list mail:

On 05-06-2014 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
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 5 years ago by kcrisman

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 5 years ago by git

• 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/8734-1 ​3d74cad 8734: rename private function, doctest it ​3180e7b Merge branch 'develop' into t/8734/ticket/8734-1 ​cec9681 8734: merge conflicts ​f702088 take back unrelated change ​2d31fb1 8734: doc cosmetics ​100c8eb Merge branch 't/8734/ticket/8734-1' into t/16007/ticket/16007 ​630ea92 16007: fix doctests; add documentation

### comment:15 follow-up: ↓ 16 Changed 5 years ago by rws

Merged in #8734. Thanks for the doctest reminder.

### comment:16 in reply to: ↑ 15 Changed 5 years ago by kcrisman

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 5 years ago by git

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

OK. Though not wrong, it wasn't what I intended, so it's changed now.

### comment:19 follow-up: ↓ 20 Changed 5 years ago by 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 - 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 seek-and-find 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 ; follow-up: ↓ 21 Changed 5 years ago by nbruin

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

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 5 years ago by nbruin

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.

Version 0, edited 5 years ago by nbruin (next)

### comment:23 in reply to: ↑ 21 Changed 5 years ago by kcrisman

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)
<ipython-input-4-2cd6ee2c70b0> 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 5 years ago by git

• 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:26 Changed 5 years ago by kcrisman

• Reviewers set to Nils Bruin, Karl-Dieter Crisman

### comment:27 follow-up: ↓ 28 Changed 5 years ago by rws

Not positive? So what's missing?

### comment:28 in reply to: ↑ 27 Changed 5 years ago by kcrisman

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 5 years ago by kcrisman

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

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

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 5 years ago by git

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

• Status changed from needs_work to needs_review

### comment:34 in reply to: ↑ 31 Changed 5 years ago by kcrisman

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.

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

Well, now there is a code point where the behaviour can be changed. So, again, is there anything missing?

### comment:36 follow-up: ↓ 38 Changed 5 years ago by kcrisman

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 5 years ago by git

• Commit changed from fe2a0f0c4c536e2ad4f5fb940ce6ee7e3a1f0c22 to 28a08aa0c6a28100d4d2e33643b0bb23b6831bb5

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

 ​7918e73 Merge branch 'develop' into t/16007/ticket/16007 ​28a08aa 16007: doctest latexification; move author info to top

### comment:38 in reply to: ↑ 36 Changed 5 years ago by rws

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 5 years ago by kcrisman

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 5 years ago by kcrisman

• Status changed from needs_review to positive_review

### comment:41 Changed 5 years ago by vbraun

• 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 5 years ago by kcrisman

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 5 years ago by git

• 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/8734-1' of trac.sagemath.org:sage into t/16007/ticket/16007 ​4add98a 16007: fix documentation examples

### comment:44 Changed 5 years ago by rws

• Status changed from needs_work to needs_review

Not all of #8734 was merged. Sorry for the inconvenience.

### comment:45 Changed 5 years ago by vbraun

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

• Dependencies set to #8734

### comment:47 Changed 5 years ago by git

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

 ​6bfa305 Merge branch 'develop' into t/16007/ticket/16007 ​a195985 8734: remove superfluous code ​67426e1 Merge branch 'u/rws/ticket/8734-1' of trac.sagemath.org:sage into t/16007/ticket/16007

### comment:48 Changed 5 years ago by kcrisman

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 non-doctest 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 5 years ago by git

• 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/8734-1 ​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/8734-1 ​a8df107 Merge branch 't/8734/ticket/8734-1' into t/16007/ticket/16007

### comment:50 Changed 5 years ago by rws

Thanks. Pulled in changes from dependencies. I'll run a patchbot on this now.

### comment:51 Changed 5 years ago by vbraun

• Branch changed from u/rws/ticket/16007 to a8df107e76527d83a87456520395ab85dbc44050
• Resolution set to fixed
• Status changed from needs_review to closed

### comment:52 Changed 4 years ago by chapoton

• Commit a8df107e76527d83a87456520395ab85dbc44050 deleted
• Description modified (diff)
Note: See TracTickets for help on using tickets.