#25936 closed enhancement (fixed)

Improvements to integrated curves and geodesics on manifolds

Reported by: gh-FlorentinJ Owned by:
Priority: major Milestone: sage-8.5
Component: geometry Keywords: manifold, geodesic, integrated curve, integration, chart
Cc: egourgoulhon, karimvanaelst Merged in:
Authors: Florentin Jaffredo Reviewers: Travis Scrimshaw, Eric Gourgoulhon
Report Upstream: N/A Work issues:
Branch: 61061c1 (Commits) Commit: 61061c152b58324f182dccc2d89a35d8fe045709
Dependencies: Stopgaps:

Description

This ticket implements some improvements regarding integrated curves on manifolds, first appearing in #22951.

It adds scipy.integrate.ode as an integration solver. The main advantage is that it supports basic event handling, making it possible to detect frontiers of charts. This allows for integration on multiple charts, as shown in this notebook, which can be particularly useful to avoid singularities, or in manifold which cannot be represented by a single chart, like in this one (not commented).

A side effect of this new method is that the time needed for the integration can be reduced (a lot!) by using objects created with fast_callable instead of symbolic expressions. This is used in this notebook to compute the shape of a geodesic in a Kerr spacetime. Speed test shows a x700 speedup compared to the default integration method "rk4_maxima".

Change History (16)

comment:1 Changed 23 months ago by git

  • Commit changed from 3512312ee75d8e158067f2166696854ee2757e95 to 65f231dd03d59c38239f3d499607fb707d907d50

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

8c5864ark4_maxima called with fast_callable
65f231dMerged with another branch already implementing all the functions

comment:2 Changed 22 months ago by git

  • Commit changed from 65f231dd03d59c38239f3d499607fb707d907d50 to 4c9732ad7523e1f72bc8a38354816397e9cdccd4

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

4c9732aBetter __reduce__

comment:3 Changed 22 months ago by gh-FlorentinJ

  • Status changed from new to needs_review

comment:4 Changed 22 months ago by gh-FlorentinJ

  • Status changed from needs_review to needs_work

Forgot some doctests after the last commit + debug print not removed

comment:5 Changed 22 months ago by git

  • Commit changed from 4c9732ad7523e1f72bc8a38354816397e9cdccd4 to f0330d2eabde2f251155f4fd3bb08bddf52cd760

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

f0330d2Fix doctests and modules

comment:6 Changed 22 months ago by gh-FlorentinJ

  • Status changed from needs_work to needs_review

comment:7 Changed 20 months ago by tscrim

Some little things:

  • You have $ in docstrings which should be `.
  • Sometimes it is better to break PEP8 80 char/line, e.g.:
    -        for ichart in set(initial_pt._coordinates.keys()).\
    -            intersection(charts):
    +        for ichart in set(initial_pt._coordinates.keys()).intersection(charts):
    
  • This is confusing to read and needs more whitespace padding on the subsequent lines:
                  res += self.plot_integrated(chart=chart,
               ambient_coords=ambient_coords, mapping=mapping, prange=prange,
               interpolation_key=interpolation_key+"_chart_"+str(i),
               include_end_point=include_end_point,
               end_point_offset=end_point_offset, verbose=verbose, color=color[i],
               style=style, label_axes=False, display_tangent=display_tangent,
               color_tangent=color_tangent, across_charts=False,
               plot_points=nb_pts, **kwds)
    
  • Please remove a lot (all) of these blanklines:
    +            sphinx_plot(fig)
    +
    +
    +
    +
    +        """
    
  • PEP8:
         def solve_across_charts(self, charts=None, step=None, solution_key=None,
    -              parameters_values=None, verbose=False):
    +                            parameters_values=None, verbose=False):
    
  • euclidean -> Euclidean.
  • Let's also Add restrictions -> Let us also add restrictions.
  • You're not always consistent with your periods/full-stops at the end of verbose messages.

Otherwise I think I am good with the current branch as it seems to work as claimed. Eric, any other comments?

comment:8 Changed 20 months ago by git

  • Commit changed from f0330d2eabde2f251155f4fd3bb08bddf52cd760 to d16555c536fddedf2f7a69657c7af5dccbfbcd39

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

687ad71Merge branch 'public/manifolds/better_integrated_curve' of git://trac.sagemath.org/sage into public/manifolds/better_integrated_curve
d16555cA little bit of cleanup for integrated curves.

comment:9 follow-up: Changed 20 months ago by tscrim

  • Reviewers set to Travis Scrimshaw

This takes care of my comment:7. Eric, do you have any comments? Otherwise I think this is good to go.

comment:10 Changed 20 months ago by git

  • Commit changed from d16555c536fddedf2f7a69657c7af5dccbfbcd39 to 61061c152b58324f182dccc2d89a35d8fe045709

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

61061c1Correct doctest + documentation in integrated curves

comment:11 in reply to: ↑ 9 Changed 20 months ago by egourgoulhon

Replying to tscrim:

This takes care of my comment:7. Eric, do you have any comments? Otherwise I think this is good to go.

Sorry for the delay in replying... I've pushed a new commit (comment:10), which corrects a doctest error as well as a pyflakes one. It also improves slightly the documentation. This is fully OK for me. Karim, do you have any further comment?

comment:12 Changed 20 months ago by egourgoulhon

  • Milestone changed from sage-8.4 to sage-8.5

comment:13 follow-up: Changed 20 months ago by tscrim

Good for me up to any comments from Karim.

comment:14 in reply to: ↑ 13 Changed 20 months ago by karimvanaelst

Replying to tscrim:

Good for me up to any comments from Karim.

I actually have no comment to make, thank you for asking and for this great improvement!

comment:15 Changed 20 months ago by tscrim

  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon
  • Status changed from needs_review to positive_review

comment:16 Changed 19 months ago by vbraun

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