Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the orbital analyser to use the Trajectory interface #3519

Merged
merged 65 commits into from
Feb 12, 2023

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Jan 30, 2023

(There is still one TODO.)

#3493

🍐

Run on (4 X 3311 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
----------------------------------------------------------------------------------------------------
Benchmark                                                          Time             CPU   Iterations
----------------------------------------------------------------------------------------------------
OrbitalElementsBenchmark/ComputeOrbitalElementsEquatorial  579610000 ns    578125000 ns            1

EXPECT_THAT(elements.mean_semimajor_axis_interval().max,
IsNear(14'910.3_(1) * Kilo(Metre)));
IsNear(14'913.5_(1) * Kilo(Metre)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes from various experiments on this test:

  • increasing the number of steps per period has no effect;
  • if downsampling is removed, the results are consistent with the old expectations, both with the old and the new OrbitalElements logic;
  • the downsampling tolerance is 10 m.

Copy link
Member Author

@pleroy pleroy Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With a tolerance of 10 m, the trajectory has 149 776 points.

With a tolerance of 1 m, the trajectory has 259 023 points, but the semimajor axis range is only 14 909.5 km .. 14 910.7 km. So we should just lower the tolerance for the orbit analysers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further investigation: at 10 cm the range becomes 14 909.9 km .. 14 910.3 km, which seems like a relevant difference given the size of the range under consideration. Lowering the tolerance further does not change the range to a precision of 100 m; in particular that range is the same in the absence of downsampling.

There is a separate issue, which is that the estimated range is highly sensitive to the value of 24 points per period (with no downsampling, 96 points gives a range of 14 909.7 km .. 14 911.0 km, and 120 points a range of 14 910.5 km .. 14 911.0 km. This is because, contrary to the apsides, we make no attempt to interpolate when estimating the extrema.

This is a problem, but it is a problem we had already (we got our arbitrary samples from the integration of motion followed by downsampling instead of getting them from the averaging of elements, but there was no attempt at interpolation either). We should now have the tools to tackle it properly, since we are already doing most of the work needed to compute the derivatives of the mean elements; but this is a matter for another pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a problem we had already

Hm, no, this does not make sense. We would not be getting maxima larger than the correct one nor minima smaller than the correct one just because of poor sampling. Is the integrator not converged enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the integrator not converged enough?

Indeed; the integrators converge only to order 1 or 2 (at least at the precisions that are relevant here), because the of the discontinuities in the higher derivatives of the downsampled trajectory.

Aside: it is clear that here we want high enough precision that a dozen points per period is not enough, and for an elliptic orbit that means we benefit from a variable stepsize.

Possibilities: Heun–Euler (but that is unsatisfactory, it is mostly cutting our losses), or lowering the downsampling tolerance (maybe to 1 mm?) until Dormand–Prince converges properly.

astronomy/orbit_analysis_test.cpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_test.cpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_test.cpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_test.cpp Outdated Show resolved Hide resolved
integrators/methods.hpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
Instant const& t_min,
Instant const& t_max) {
Time const Δt = t_max - t_min;
Instant const t0 = t_min + Δt / 2;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call this t_mid?

astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
astronomy/orbital_elements_body.hpp Outdated Show resolved Hide resolved
ksp_plugin/integrators.hpp Outdated Show resolved Hide resolved
@pleroy pleroy added the LGTM label Feb 12, 2023
@pleroy pleroy merged commit 30e2838 into mockingbirdnest:master Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants