#28707 closed enhancement (fixed)

More control on the numerical ODE solver for integrated curves and geodesics

Reported by: egourgoulhon Owned by:
Priority: major Milestone: sage-9.0
Component: geometry Keywords: manifolds, integrated_curve, geodesic
Cc: gh-FlorentinJ, karimvanaelst Merged in:
Authors: Eric Gourgoulhon Reviewers: Florentin Jaffredo
Report Upstream: N/A Work issues:
Branch: aca08a8 (Commits) Commit: aca08a898e22586d2d9fcb4f29e830cc788f165f
Dependencies: Stopgaps:

Description (last modified by egourgoulhon)

This ticket allows to pass extra parameters to the method IntegratedCurve.solve() in order to control the numerical ODE solver. In particular, this allows to fix an issue with the odeint solver by increasing its precision via some tolerance parameters (see comment:1 for details).

Besides, this ticket makes the odeint solver (introduced in #25936) the default one, instead of rk4_maxima, since the former is much faster than the latter, as already noticed in #25936. The method name 'ode_int' has been changed to 'odeint', in order to match with desolve_odeint() and the SciPy name (for backward compatibility, method='ode_int' is still accepted though).

Moreover, this ticket performs some improvement of the documentation of integrated curves.

Attachments (2)

test_geod_bad.png (20.1 KB) - added by egourgoulhon 11 months ago.
test_geod.png (18.5 KB) - added by egourgoulhon 11 months ago.

Download all attachments as: .zip

Change History (13)

Changed 11 months ago by egourgoulhon

Changed 11 months ago by egourgoulhon

comment:1 Changed 11 months ago by egourgoulhon

In Sage 8.9, when integrating a null geodesic in Schwarzschild spacetime with an impact parameter close to critical (all details are in this notebook), the solution obtained with solve(method='ode_int') (green in the figure below) differs significantly from those obtained by means of other solvers, for instance the rkf45 solver (red in the figure below):

The geodesic arises from the upper right ((x,y) approx. (10, 5)), circles twice around the black hole (the grey area) and then departs away to the lower left (rkf45 "exact" solution) or to the lower right (ode_int bad solution). This issue is due to the default values of 1.49012e-8 for the parameters rtol and atol of scipy.integrate.odeint being not sufficient in the present case. Now, with the Sage 8.9 implementation of IntegratedCurve.solve(), there is no way to change the values of rtol and atol. Thanks to the new argument **control_param introduced here this becomes possible. Morevoer, the default values of rtol and atol are changed to 1.e-10. Accordingly, the curves obtained from the odeint and rkf45 become almost undistinguishable:

See the notebook for a detailed study of the agreement between odeint and the other solvers when using the code of this ticket branch.

Last edited 10 months ago by egourgoulhon (previous) (diff)

comment:2 Changed 11 months ago by egourgoulhon

  • Branch set to public/manifolds/geod_integrator_options
  • Commit set to 511e26d6b637c8fb85c0112c8a5185d24c105672
  • Description modified (diff)

New commits:

7b44ef4Add control parameters to IntegratedCurve.solve()
e31f6cbExpose methods of scipy.integrate.ode in IntegratedCurve.solve()
0cc2077Change default method in IntegratedCurve.solve() from rk4_maxima to odeint
bd1392bImprove documentation of integrated curves
511e26dMore improvements in the documentation of integrated curves

comment:3 Changed 11 months ago by egourgoulhon

  • Cc gh-FlorentinJ karimvanaelst added
  • Status changed from new to needs_review

comment:4 Changed 11 months ago by git

  • Commit changed from 511e26d6b637c8fb85c0112c8a5185d24c105672 to 2e918f44c02dcf185126ae3865c6769f446ceddc

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

2e918f4Correct some doctests after the change of default method in IntegratedCurve.solve()

comment:5 Changed 10 months ago by git

  • Commit changed from 2e918f44c02dcf185126ae3865c6769f446ceddc to aca08a898e22586d2d9fcb4f29e830cc788f165f

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

aca08a8Merge branch public/manifolds/geod_integrator_options into Sage 9.0.beta5

comment:6 Changed 10 months ago by egourgoulhon

The above commit fixes a merge conflict with Sage 9.0.beta5, due to #28669.

comment:7 follow-up: Changed 10 months ago by gh-FlorentinJ

  • Reviewers set to gh-FlorentinJ
  • Status changed from needs_review to positive_review

Seems perfect to me. Many thanks Éric.

(The failed plugins are only there because of the merge with #28669, no issue here)

Last edited 10 months ago by gh-FlorentinJ (previous) (diff)

comment:8 Changed 10 months ago by egourgoulhon

  • Reviewers changed from gh-FlorentinJ to Florentin Jaffredo

comment:9 in reply to: ↑ 7 Changed 10 months ago by egourgoulhon

Replying to gh-FlorentinJ:

Seems perfect to me. Many thanks Éric.

Thank you for the review!

(The failed plugins are only there because of the merge with #28669, no issue here)

I think it's rather an issue with this patchbot: there are many errors and all reports say that they happen "on 0 non-empty lines". The "0" is highly suspicious here...

comment:10 Changed 10 months ago by gh-FlorentinJ

The diff for the failed plugin only contain 2 lines:

--- 9.0.beta5
+++ 9.0.beta5 + #28707

Isn't that the reason it failed ?

comment:11 Changed 10 months ago by vbraun

  • Branch changed from public/manifolds/geod_integrator_options to aca08a898e22586d2d9fcb4f29e830cc788f165f
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.