Edited doc strings in geomstats/integrator.py #1917
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1917 +/- ##
==========================================
+ Coverage 90.29% 90.41% +0.12%
==========================================
Files 143 143
Lines 13407 13407
==========================================
+ Hits 12105 12120 +15
+ Misses 1302 1287 -15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for this first contribution @dralston78, and welcome to the community!
In fact, the design of @nguigs is very generic and works to solve systems of first order ODEs (or a first-order ODE) resulting from n-th order ODEs (both autonomous and non-autonomous).
Everything is controlled by the force function.
The first dimension of the state array tells about the order of the original ODE. Right now the docstrings are in fact misleading, as they suggests only two variables can be solved for (position and velocity).
Though this design is generic, we use this code to solve an (autonomous) second-order ODE (see geodesic_equation in geomstats.geometry.connection.Connection). This explains why some docstrings have shape=[2, dim] instead of shape=[n, dim], where n is the order of the original ODE. Returns are also inconsistent (it returns a new_state). This definitely needs to be updated and made consistent. Would you be willing to do so?
I've also seen you've done the leapfrog_step implementation. I would be very glad if you open a PR with it (I can help you out with the tests).
Looking forward to see your future contributions!
|
Thanks for your feedback @luisfpereira! I see what you mean, thanks for clarifying that with me. So I think the correct phrasing is that this method solves an
|
|
I fully agree with your suggestions for the docstrings. So, feel free to do them in this PR. As you suggest, I think it is better to do the leapfrog in another PR. Thanks again! I'll be waiting for your contributions. |
Checklist
Description
I clarify some of the doc strings in the integrator.py file. I also fixed a few typos I noticed.
Issue
As I understand, the implemented functions perform numerical integration on first order autonomous systems of ODEs, i.e. integrating an n-dimensional vector field. The functions that are not implemented (leapfrog, symplectic Euler) perform numerical integration on second order autonomous systems of ODEs, i.e. integrating an n-dimensional acceleration field (by creating a system of first order ODEs). I wanted to update the doc strings to clarify these distinctions.
Additional context
This is my first time making a pull request on Github, as well as my first time contributing to geomstats, so my apologies in advance if I misunderstood or missed something!