Opened 2 years ago
Closed 2 years ago
#25936 closed enhancement (fixed)
Improvements to integrated curves and geodesics on manifolds
Reported by:  ghFlorentinJ  Owned by:  

Priority:  major  Milestone:  sage8.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 2 years ago by
 Commit changed from 3512312ee75d8e158067f2166696854ee2757e95 to 65f231dd03d59c38239f3d499607fb707d907d50
comment:2 Changed 2 years ago by
 Commit changed from 65f231dd03d59c38239f3d499607fb707d907d50 to 4c9732ad7523e1f72bc8a38354816397e9cdccd4
Branch pushed to git repo; I updated commit sha1. New commits:
4c9732a  Better __reduce__

comment:3 Changed 2 years ago by
 Status changed from new to needs_review
comment:4 Changed 2 years ago by
 Status changed from needs_review to needs_work
Forgot some doctests after the last commit + debug print not removed
comment:5 Changed 2 years ago by
 Commit changed from 4c9732ad7523e1f72bc8a38354816397e9cdccd4 to f0330d2eabde2f251155f4fd3bb08bddf52cd760
Branch pushed to git repo; I updated commit sha1. New commits:
f0330d2  Fix doctests and modules

comment:6 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:7 Changed 2 years ago by
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/fullstops 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 2 years ago by
 Commit changed from f0330d2eabde2f251155f4fd3bb08bddf52cd760 to d16555c536fddedf2f7a69657c7af5dccbfbcd39
comment:9 followup: ↓ 11 Changed 2 years ago by
 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 2 years ago by
 Commit changed from d16555c536fddedf2f7a69657c7af5dccbfbcd39 to 61061c152b58324f182dccc2d89a35d8fe045709
Branch pushed to git repo; I updated commit sha1. New commits:
61061c1  Correct doctest + documentation in integrated curves

comment:11 in reply to: ↑ 9 Changed 2 years ago by
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 2 years ago by
 Milestone changed from sage8.4 to sage8.5
comment:13 followup: ↓ 14 Changed 2 years ago by
Good for me up to any comments from Karim.
comment:14 in reply to: ↑ 13 Changed 2 years ago by
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 2 years ago by
 Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Eric Gourgoulhon
 Status changed from needs_review to positive_review
comment:16 Changed 2 years ago by
 Branch changed from public/manifolds/better_integrated_curve to 61061c152b58324f182dccc2d89a35d8fe045709
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
rk4_maxima called with fast_callable
Merged with another branch already implementing all the functions