Add M_to_nu_elliptic #1398
Add M_to_nu_elliptic #1398
Conversation
MLopez-Ibanez
force-pushed
the
M_to_nu_elliptic
branch
from
faad3c4
to
c733704
Compare
MLopez-Ibanez
force-pushed
the
M_to_nu_elliptic
branch
from
c733704
to
6e29a81
Compare
Codecov Report
@@ Coverage Diff @@
## main #1398 +/- ##
=======================================
Coverage 91.96% 91.96%
=======================================
Files 82 82
Lines 4342 4346 +4
Branches 362 362
=======================================
+ Hits 3993 3997 +4
Misses 259 259
Partials 90 90
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Not sure about what to do with this pull request, for the reasons stated below.
On one hand, as I stated in #1013, we don't have generic "M from/to nu" on purpose since, as I painfully discovered in #908 (comment), the mean anomaly is far from being a well-defined quantity with a clear geometrical or physical meaning for all eccentricity regimes. Having a M_to_nu_elliptic invites the question of "why not having M_to_nu_{parabolic,hyperbolic} as well. In any case, we need to document this better, both for users and for potential contributors.
On the other hand, M_to_nu_elliptic is not a generic converter, but is only meant to be used for elliptic orbits. There is no eccentricity check though.
And finally, you mention that this is "slightly faster" than calling E_to_nu(M_to_E()), probably because of similar reasons as the ones you stated in #1360 (comment). But can we have a measurement of how much faster it is, so we can have an idea of the performance gain?
If we were to merge this, I'd vote to:
- Move a function to a different module, like
poliastro.util, to make it clear that this is a convenience function. - Add a short sentence to the docstring pointing out that parabolic and hyperbolic versions are not provided on purpose, and linking to Add docs on converting mean anomaly to true anomaly #1013
- Make it raise a
ValueErrorif the eccentricity is out of range.
What do you think?
from poliastro.twobody.angles import M_to_nu_elliptic, E_to_nu, M_to_E
from astropy import units as u
%timeit [ M_to_nu_elliptic(M=x * u.deg, ecc=0.1 * u.one) for x in range(0,180)]
%timeit [ E_to_nu(M_to_E(M=x * u.deg, ecc=0.1 * u.one), ecc=0.2 * u.one) for x in range(0,180)]In [2]: %timeit [ M_to_nu_elliptic(M=x * u.deg, ecc=0.1 * u.one) for x in range(0,180)]
52.8 ms ± 3.1 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
In [3]: %timeit [ E_to_nu(M_to_E(M=x * u.deg, ecc=0.1 * u.one), ecc=0.2 * u.one) for x in range(0,180)]
117 ms ± 19.6 ms per loop (mean ± std. dev. of 7 runs, 10 loops each) |
I didn't provide them because I didn't use them. But I noticed that poliastro itself does convert from M to nu for parabolic and hyperbolic in at least two places: |
|
Yep, we are forced to do that because 2 public NASA APIs use M as the anomaly for potentially parabolic and hyperbolic objects as well. I'm sure there's a good reason for it, but I'm not sure which. |
|
Ah sorry, that belongs to the propagator itself (didn't have my morning coffee yet). I wonder if the performance gain also happens if the caller is also jitted? |
To be clear, the ambiguity appears mainly in the near-parabolic region (not so uncommon for comets), and my beef is with calling M "an angle" for non-elliptic orbits. |
I would assume that if numba does inlining (or when numba does inlining), then there should not be any measurable difference when using just the core versions. But the caller code in orbit.py is not using the core versions. |
Sure. Should the core version also move to poliastro.core.util ?
What should the sentence say?
What would be an acceptable range? This is something that could be done for the non-core version for extra safety. However, note that M_to_E does not have such a check (nor any other M_to_X function). |
No description provided.