Opened 7 years ago

Closed 6 years ago

#16581 closed enhancement (fixed)

Added tides based desolvers

Reported by: mmarco Owned by:
Priority: major Milestone: sage-6.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 mmarco

  • 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 mmarco

  • Commit set to fba3457b8edb69479bdc46e80b168227080d3215
  • Status changed from new to needs_review

New commits:

31dfa87cleaned mintides desolver, separated file generators to the tides directory
addab3fchanged authors
a6c12cfadded c files
6fa0e48fully documented version
8bee645Add tides as package.
99a99b7Merge branch 'tideslib' into tides
f3bbadccleaned dependencdy on .c and .h files
fba3457Cleaned desolvers based in tides

comment:3 Changed 7 years ago by mmarco

  • Keywords sd59 added

comment:4 Changed 7 years ago by vdelecroix

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

Last edited 7 years ago by vdelecroix (previous) (diff)

comment:5 Changed 7 years ago by mmarco

Yeah, sorry for the long list of commits, i made a lot of trial-and-error 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 vdelecroix

Yes!!! I just learn it today: git rebase -i see: rewriting history

comment:7 Changed 7 years ago by vdelecroix

But be careful: keep distinct commits for distinct tickets!

comment:8 Changed 7 years ago by git

  • Commit changed from fba3457b8edb69479bdc46e80b168227080d3215 to 8b12bac74d036ab7546c63f9a8de2b8d0ef0f9e0

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

8b12bacsolved wrong import statement

comment:9 Changed 7 years ago by vbraun

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 mmarco

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 vbraun

About the first, you can use the cython() command in Sage to compile a new cython extension module at run-time. 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 mmarco

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 iMark

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 vbraun

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 trade-off 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 hard-coding "/" as directory separator
  • less blank lines (see pep8). Double blanks only to separate class definitons and top-level 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 vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:16 Changed 6 years ago by git

  • Commit changed from 8b12bac74d036ab7546c63f9a8de2b8d0ef0f9e0 to ae8c869bc35ada66013d731be5eb003c3cf396fd

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

e192f0aReviewer's changes on mintides
ae8c869Reviewer changes in decolve_tides_mpfr

comment:17 Changed 6 years ago by mmarco

Made the suggested changes.

comment:18 Changed 6 years ago by mmarco

  • Cc vbraun added

comment:19 Changed 6 years ago by mmarco

  • Cc vdelecroix added

comment:20 Changed 6 years ago by iMark

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 Sage-tides 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 vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:22 Changed 6 years ago by vbraun

  • 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 1e-5
Expected:
    [[0.000000000000000,
    0.800000000000000,
    0.000000000000000,
    0.000000000000000,
    1.22474487139159],
    [314.159265358979,
    0.800000000028622,
    -5.91973525754241e-9,
    7.56887091890590e-9,
    1.22474487136329]]
Got:
    [[0.000000000000000,
      0.800000000000000,
      0.000000000000000,
      0.000000000000000,
      1.22474487139159],
     [314.159265358979,
      0.800000000028628,
      -5.92006829669423e-9,
      7.56929363632253e-9,
      1.22474487136328]]
**********************************************************************

comment:23 Changed 6 years ago by git

  • Commit changed from ae8c869bc35ada66013d731be5eb003c3cf396fd to 0ff9e7f48bc164c95cece98b3c3d18bfcdbd7ef6

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

0ff9e7fFixed numerical tolerance in doctests

comment:24 Changed 6 years ago by mmarco

  • Status changed from needs_work to needs_review

Fixed

comment:25 Changed 6 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:26 Changed 6 years ago by vbraun

  • Branch changed from u/mmarco/ticket/16581 to 0ff9e7f48bc164c95cece98b3c3d18bfcdbd7ef6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.