Opened 7 years ago
Closed 6 years ago
#16581 closed enhancement (fixed)
Added tides based desolvers
Reported by:  mmarco  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  numerical  Keywords:  sd59 
Cc:  vbraun, vdelecroix  Merged in:  
Authors:  Miguel Marco  Reviewers:  Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  0ff9e7f (Commits)  Commit:  0ff9e7f48bc164c95cece98b3c3d18bfcdbd7ef6 
Dependencies:  #16578, #16579  Stopgaps: 
Description
This patch adds two solvers for differential equations (desolve_mintides and desove_tides_mpfr) based on the TIDES library.
Change History (26)
comment:1 Changed 7 years ago by
 Branch set to u/mmarco/ticket/16581
 Created changed from 06/28/14 07:10:05 to 06/28/14 07:10:05
 Modified changed from 06/28/14 07:10:05 to 06/28/14 07:10:05
comment:2 Changed 7 years ago by
 Commit set to fba3457b8edb69479bdc46e80b168227080d3215
 Status changed from new to needs_review
comment:3 Changed 7 years ago by
 Keywords sd59 added
comment:4 Changed 7 years ago by
Hi Miguel,
Why did you include tides in source instead of making a spkg? The sources of packages are not under git management. You would better follow the instructions at http://www.sagemath.org/doc/developer/packaging.html.
EDIT: I see it is at #16578... sorry for the noise
COMMENT: in the list of commit above it is very hard to see which are the one associated to that ticket!!
Vincent
comment:5 Changed 7 years ago by
Yeah, sorry for the long list of commits, i made a lot of trialanderror and refactoring before having this ready.... i guess there could be a way to tell git to merge all those commits into one.
comment:6 Changed 7 years ago by
Yes!!! I just learn it today: git rebase i
see: rewriting history
comment:7 Changed 7 years ago by
But be careful: keep distinct commits for distinct tickets!
comment:8 Changed 7 years ago by
 Commit changed from fba3457b8edb69479bdc46e80b168227080d3215 to 8b12bac74d036ab7546c63f9a8de2b8d0ef0f9e0
Branch pushed to git repo; I updated commit sha1. New commits:
8b12bac  solved wrong import statement

comment:9 Changed 7 years ago by
You should use the CC / CFLAGS / LDFLAGS environment variables (CXX for C++).
IMHO the integration is a bit underwhelming. Its ok for an optional package, I guess. But better solutions would be
 Use the
cython()
command to dynamically generate a function that is callable from Python, without parsing strings anywhere. Leaks a bit of memory from the imports.  Use
fast_callable(..., domain=RealField(prec))
to dynamically construct the expression tree and only have a single Cython class that can call tides as appropriate.
Do you have any plans in that direction?
comment:10 Changed 7 years ago by
I am not sure if i understand your second suggestion. About the first one, it could be a good idea for this part (the desolvers). But it should be substitute the .c files generators in #16579, since those .c files are useful by themselves (one of the use cases of TIDES is to generate those files qith Mathematica and then tweak them by hand to get slightly different solutions, or extra information from them).
If i understand correctly, you are suggesting to substitute the call to gcc by some small cython object. Is that correct?
comment:11 Changed 7 years ago by
About the first, you can use the cython()
command in Sage to compile a new cython extension module at runtime. With the magic comment #cfile foo.c
(see cython?
) you can even include C sources. So it should be rather easy to implement.
The second suggestion is to not compile stuff whenever you solve a differential equation (really, nobody else does that). You can just compile it once and evaluate the function/derivatives dynamically.
comment:12 Changed 7 years ago by
I am not sure if the tides design allows that kind of workflow. I will ask the developers about it. Apparently, in the world of specialized de solvers, compiling write and compile a program for each particular de is the usual (which was very surprising for me when I found out)
comment:13 Changed 6 years ago by
Hi. I'm Marcos Rodriguez, a developer of TIDES.
Thank you very much for the reviewing effort.
In the numerical computation of a differential equation, the bottleneck is at the computation of the taylor series coefficients (code generated by the parser). Therefore, this scheme should be as much optimized as possible via compiler. So, a dynamical function call for this part would slow down the computation significantly.
Besides, the original goal of MathTIDES (the parser written in Mathematica) was the generation of C of Fortran code which compilation and execution is independent of the symbolic preprocessor (Mathematica in this case) so that it can be tweaked or integrated in other C or Fortran codes for different purposes.
We think that the natural extension is to compile, execute and display this code in a free symbolic preprocessor.
comment:14 Changed 6 years ago by
Thanks for posting here!
Its of course faster to do anything numerical in compiled code. But there is a reason why we don't always do that (e.g. when plotting functions). At the end of the day it is just a tradeoff between speed in a single example vs. preprocessing time and ease of use. Imagine you want to solve a million ODEs, each only a short bit into the future. In any case, this need not be done on this ticket.
Miguel:
 use subprocess.check_call instead of os.system to catch potential errors
 "which" is not posix, use "command v"
 Use sage's tmp_dir instead of mkdtmp
 os.path.join instead of hardcoding "/" as directory separator
 less blank lines (see pep8). Double blanks only to separate class definitons and toplevel functions.
 You should link to Sage's mpir/mpfr instead of the system default. Use
gcc I$SAGE_ROOT/local/include L$SAGE_ROOT/local/lib
for header and library search paths.
comment:15 Changed 6 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:16 Changed 6 years ago by
 Commit changed from 8b12bac74d036ab7546c63f9a8de2b8d0ef0f9e0 to ae8c869bc35ada66013d731be5eb003c3cf396fd
comment:17 Changed 6 years ago by
Made the suggested changes.
comment:18 Changed 6 years ago by
 Cc vbraun added
comment:19 Changed 6 years ago by
 Cc vdelecroix added
comment:20 Changed 6 years ago by
Hello.
Do you think that after the changes, a possitive revision can be given (with possible minor changes), or it would need a complete rewrite. The reason for me to ask is that we are writing a paper about TIDES and integration using Taylor Method and we would like to include comparisons using Sagetides and anounce the Sage version of the package as a completely free version of the software.
Thank you very much.
comment:21 Changed 6 years ago by
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
comment:22 Changed 6 years ago by
 Status changed from positive_review to needs_work
You want # abs tol
File "src/sage/calculus/desolvers.py", line 1617, in sage.calculus.desolvers.desolve_mintides Failed example: sol # optional tides # abs rel 1e5 Expected: [[0.000000000000000, 0.800000000000000, 0.000000000000000, 0.000000000000000, 1.22474487139159], [314.159265358979, 0.800000000028622, 5.91973525754241e9, 7.56887091890590e9, 1.22474487136329]] Got: [[0.000000000000000, 0.800000000000000, 0.000000000000000, 0.000000000000000, 1.22474487139159], [314.159265358979, 0.800000000028628, 5.92006829669423e9, 7.56929363632253e9, 1.22474487136328]] **********************************************************************
comment:23 Changed 6 years ago by
 Commit changed from ae8c869bc35ada66013d731be5eb003c3cf396fd to 0ff9e7f48bc164c95cece98b3c3d18bfcdbd7ef6
Branch pushed to git repo; I updated commit sha1. New commits:
0ff9e7f  Fixed numerical tolerance in doctests

comment:25 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:26 Changed 6 years ago by
 Branch changed from u/mmarco/ticket/16581 to 0ff9e7f48bc164c95cece98b3c3d18bfcdbd7ef6
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
cleaned mintides desolver, separated file generators to the tides directory
changed authors
added c files
fully documented version
Add tides as package.
Merge branch 'tideslib' into tides
cleaned dependencdy on .c and .h files
Cleaned desolvers based in tides