#8950: new function for sage.calculus.desolve module
------------------------------+---------------------------------------------
Reporter: uri | Owner: burcin
Type: enhancement | Status: needs_work
Priority: minor | Milestone: sage-4.5.2
Component: calculus | Keywords: desolve
Author: Oriol Castejon | Upstream: N/A
Reviewer: | Merged:
Work_issues: |
------------------------------+---------------------------------------------
Comment(by uri):
Thanks pang, I think the changes you've done are great! Here's my opinion:
Replying to [comment:12 pang]:
> 1. I'm concerned about using the variable 't' if the argument ivar is
not set explicitely. Is it safe to assume that nobody would use the name
't' for a dependent variable? Is it safe to assume that everybody would
use the name 't' for the independent variable?
Yes, I think you're right, it's not safe to use the name 't' as default
for the independent variable.
> I would prefer either of these two approaches:
>
> * Always require the parameter ivar
> * Use some code like in `desolve_system_rk4` to try to guess the
independent var:
I prefer the second approach, because in a lot of cases the independent
variable is in fact (almost) meaningless. Besides, the code you've written
is great, and I think it should work perfectly (well I've made a small
correction, I'll explain later).
> 2. I'd use:
>
> {{{
> J=diff(des,dvars)
> }}}
>
> instead of
>
> {{{
> J=jacobian(des,dvars)
> J=J[0][0]
> }}}
I agree, I just didn't think about that.
> 3. In my opinion, the parameters to ``odeint`` mu and ml should not be
used. Those parameters do not make sense if you don't pass Dfun as an
argument. But please correct me if I'm wrong.
Again, totally agree.
> 4. I've introduced a few ``is_SymbolicVariable(dvars)`` tests. I think
the test ``len(dvars)==0`` is a lousy test for a Symbolic Variable.
I still don't really understand why you don't like the test
``len(dvars)==0``, but it's true that the ``is_SymbolicVariable(dvars)``
test is more elegant. By the way, the first ``len(dvars)==0`` test is
still there; is there any reason why the other one cannot be used?
> 5. The documentation wasn't building correctly. Sphinx is a bit rigid,
and we need to follow the indentation rules, etc. I've fixed that already.
Thanks, I didn't know that.
I attach a new patch (trac_8950_odeint_4.patch), because I made some small
changes:
1. In the documentation, the description of 'ivar' said ' default is t',
which is not anymore, so I changed it to just 'optional'.
2. In one of the examples there was a mistake, because it used the time
vector 'times' which was defined in a previous example and not the one
that was defined in it, which was called 't'.
3. At the beggining there was:
{{{
if ivar==None:
if len(dvars)==0 or len(dvars)==1:
all_vars = set(des.variables())
else:
...
}}}
However, in the case that ``len(dvars)==1`` is true, 'des.variables()'
will give an error. So instead I wrote:
{{{
if ivar==None:
if len(dvars)==0 or len(dvars)==1:
if len(dvars)==1:
des=des[0]
dvars=dvars[0]
all_vars = set(des.variables())
else:
...
}}}
As the redefinition 'dvars=dvars[0]' was already done afterwards, I moved
it to here for simplicity.
Please, let me know your opinion. And thanks again for your work!
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/8950#comment:14>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
