Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#16007 closed defect (fixed)

give solution constants of ODEs unique names

Reported by: rws Owned by:
Priority: major Milestone: sage-6.3
Component: calculus Keywords:
Cc: kcrisman Merged in:
Authors: Ralf Stephan Reviewers: Nils Bruin, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: a8df107 (Commits) Commit:
Dependencies: #8734 Stopgaps:

Description (last modified by chapoton)

(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.

Change History (52)

comment:1 Changed 6 years ago by kcrisman

  • Cc kcrisman added

comment:2 Changed 6 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 6 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 6 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:

ee630f5add %c,%k1,%k2 to recognized maxima objects

comment:5 Changed 6 years ago by zimmerma

seems to be a duplicate of #9421.

Paul

comment:6 Changed 6 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 6 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 6 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 6 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

OK. Then what remains is a warning after the second line. Could you please supply a good wording?

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

comment:12 follow-up: 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
>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 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:

1e9b54b8734: more doctests fixed
06b95288734: fix doctests
6fffc21Merge branch 'develop' into t/8734/ticket/8734-1
3d74cad8734: rename private function, doctest it
3180e7bMerge branch 'develop' into t/8734/ticket/8734-1
cec96818734: merge conflicts
f702088take back unrelated change
2d31fb18734: doc cosmetics
100c8ebMerge branch 't/8734/ticket/8734-1' into t/16007/ticket/16007
630ea9216007: fix doctests; add documentation

comment:15 follow-up: 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:

92a3fa316007: 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: 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: Changed 5 years ago by nbruin

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

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 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.

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.

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

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:

884bc6816007: give advice how to use constants

comment:25 Changed 5 years ago by rws

Please don't forget to add your names.

comment:26 Changed 5 years ago by kcrisman

  • Reviewers set to Nils Bruin, Karl-Dieter Crisman

comment:27 follow-up: 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: 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:

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

7918e73Merge branch 'develop' into t/16007/ticket/16007
28a08aa16007: doctest latexification; move author info to top

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

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

2c680268734: adapt doctest to recent change of output
793bb058734: reraise in "other" cases with bare "raise"
b5857b7Merge branch 'u/rws/ticket/8734-1' of trac.sagemath.org:sage into t/16007/ticket/16007
4add98a16007: 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

  • Commit changed from 4add98a4e671996e859bb1c008fd61909e982c20 to 67426e10a84f9f73ad8c38d63872b3debb0f794a

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

6bfa305Merge branch 'develop' into t/16007/ticket/16007
a1959858734: remove superfluous code
67426e1Merge 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:

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
a8df107Merge 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.