Conversation
|
Thanks for the contribution. It's going to take a while to review because (1) it requires a functioning brain and (2) we are in the middle of a move. In the meantime, can you fix the lint/compilation errors? |
|
I'm not sure how to fix the |
|
@eggrobin: Can you take a look at the @rnlahaye: I'll try to do a first pass over the PR tomorrow, but it won't be particularly deep. I'd like to read the papers you linked, but that will take some time. |
pleroy
left a comment
There was a problem hiding this comment.
A very shallow first review (I didn't look at the test) to get things going. Apologies for the million nitpicks.
documentation/bibliography.bib
Outdated
| @@ -1,4 +1,4 @@ | |||
| | |||
|
|
|||
There was a problem hiding this comment.
I'm not sure; I checked in a hex editor and the first few bytes of the files were identical.
documentation/bibliography.bib
Outdated
| author = {Clenshaw, C. W. and Norton, H. J.}, | ||
| publisher = {Oxford University Press}, | ||
| date = {1963}, | ||
| doi = {10.2/3}, |
There was a problem hiding this comment.
The DOI should be 10.1093/comjnl/6.1.88 I think.
| pages = {107--115}, | ||
| title = {Matrix formulation of the Picard method for parallel computation}, | ||
| volume = {29}, | ||
| } |
There was a problem hiding this comment.
Let's add the DOI 10.1007/BF01232802.
| @article{Fukushima1997, | ||
| author = {Fukushima, Toshio}, | ||
| date = {1997}, | ||
| journaltitle = {Astronomical Journal v. 113, p. 1909-1914 (1997)}, |
There was a problem hiding this comment.
Remove volume, pages, date from this.
| pages = {1909--1914}, | ||
| title = {Picard iteration method, Chebyshev polynomial approximation, and global numerical integration of dynamical motions}, | ||
| volume = {113}, | ||
| } |
| DependentVariablesFromMatrixRow<ODE>(Xⁱ_, Xⁱ_.rows() - 1)); | ||
| } else { | ||
| // We failed to converge. | ||
| return absl::Status(absl::StatusCode::kFailedPrecondition, |
There was a problem hiding this comment.
Would it make sense to use this status? There is a magic long distance coupling where the status that gets returned here determines the explanation given to the user in the UI, so we must make sure that we pick the right status.
There was a problem hiding this comment.
The comment of that status is "The integration may be retried with the same arguments and progress will happen", which is not the case here—if the iteration doesn't converge on some interval, it will repeatedly fail to converge unless the interval is made shorter.
| } | ||
| } | ||
|
|
||
| return absl::OkStatus(); |
There was a problem hiding this comment.
Note that our integrators are generally peppered with RETURN_IF_STOPPED so that we don't continue integrating if the user changed their mind. If you plan on integrating this in Principia, these statements will be needed.
There was a problem hiding this comment.
Added some RETURN_IF_STOPPED.
| Time const& step, ЧебышёвPicardIterator const& integrator) | ||
| : FixedStepSizeIntegrator<ODE>::Instance(problem, append_state, step), | ||
| integrator_(integrator), | ||
| t_(), |
There was a problem hiding this comment.
I'd omit this field, we generally don't list default initializations.
| ᵝT(i, j) = 2 * τᵢ * ᵝT(i, j - 1) - ᵝT(i, j - 2); | ||
|
|
||
| // Make sure the zeroes are actually zero. | ||
| if (std::abs(ᵝT(i, j)) < 1e-14) ᵝT(i, j) = 0; |
There was a problem hiding this comment.
{} around the statement that follows the if.
More importantly, we are not going to like the magic number 1e-14 here. I don't understand the algorithm, so I'm not sure what to recommend, but these kinds of tests are disasters waiting to happen.
There was a problem hiding this comment.
I removed the line; it wasn't actually necessary.
serialization/integrators.proto
Outdated
| optional CompositionMethod composition_method = 2; // Only for SRKN. | ||
|
|
||
| // Only for CHEBYSHEV_PICARD. | ||
| optional ChebyshevPicardIterationParams chebyshev_picard_params = 3; |
There was a problem hiding this comment.
Since some of these params clearly belong to the instance, I'm not sure if this field belongs here.
There was a problem hiding this comment.
I've removed the proto params (and the rest of the serialization code) for now; I'll add them back in a future PR.
(I don't want to add proto enums until I know what
| std::int64_t j = 0; | ||
| typename ODE_::DependentVariables y; | ||
| for_all_of(y).loop([&j, &matrix, row](auto& yⱼ) { | ||
| yⱼ = matrix(row, j++) * si::Unit<std::remove_reference_t<decltype(yⱼ)>>; |
There was a problem hiding this comment.
It is always a bit sad when we have to drop the units like that. It happens, in flight plan optimization or in Newhall. Long ago all the integrators used to do that too; this is fine for now.
I wonder though, what would the units of those matrices be, and which matrix operations are used on them? The rows seem to be dependent variables multiplied by some scalar? Are there any matrix multiplications or matrix-vector multiplications happening? (EDIT: The answer to the last question is yes, e.g., ᵝT * ᵝW. But ᵝW is diagonal and dimensionless.)
There was a problem hiding this comment.
I think all the matrices defined in the ЧебышёвPicardIterator constructor are dimensionless (as they don't depend on the ODE at all, only on
The update operation is
-
$y'$ is an$(M+1)\times n$ matrix where each row is a derivative at some time$t$ and each column has dimension of the corresponding element ofDependentVariableDerivatives. -
$C_xX_0$ is a$(M+1)\times n$ matrix where each row is a copy of the initial state$y(t_0)$ and each column has dimension of the corresponding element ofDependentVariables.
So each column of DependentVariableDifferences, multiplying this by the dimensionless
If there was a FixedMatrix variant which accepted different dimensions for each column this would probably work; in that case I wouldn't need to divide by Second in the update step.
|
I couldn’t think of a good way to test a fix to the GitHub Actions script, so I pushed changes to the check-iwyu check in here. It looks like it now manages to work well enough to tell you what changes in the log, though somehow the messages don’t show up in the diff (but I don’t know if that’s a bug on my side, GitHub having trouble with non-ASCII paths, or the fancy new GitHub UI not showing things where I expect them). I’ll cherry-pick those changes into an other PR. |
documentation/bibliography.bib
Outdated
| volume = {116}, | ||
| } | ||
|
|
||
| @article{FeaginNarcozy1983, |
There was a problem hiding this comment.
It looks like the author is called Nacozy, no r?
|
|
||
| // Set the current state to the final state we appended. | ||
| current_state = | ||
| State(t_[Xⁱ_.rows() - 1], |
There was a problem hiding this comment.
It looks like everything is dropped down to single precision (well, that’s going to be double, but single in the sense of not being DoublePrecision) at each step.
This is typically disastrous for long-term integration with the kinds of methods we currently have; for many methods compensated summation of increments computed at each step is enough, for some (the multistep methods, see #1184) full double-double additions are needed. I don’t know how this one fares, but at least it looks like the independent variable is being straightforwardly incremented, so it will get its full dose of roundoff accumulation.
This is probably not immediately actionable though. The right way to figure this out would be to add this method to https://github.com/mockingbirdnest/Principia/blob/master/mathematica/integrator_plots.cpp; the before/after of #1184 was striking:


There was a problem hiding this comment.
I am curious to see what the plots show. I'll add this method to integrator_plots.cpp after I implement the second-order version.
| @@ -0,0 +1,461 @@ | |||
| #include "integrators/чебышёв_picard_iterator.hpp" | |||
There was a problem hiding this comment.
I can’t immediately find a discussion of the convergence order in [Mac15]. This is usually something we test about our fixed-step integrators, though since this is driven by a stopping criterion, you would have to count actual evaluations rather than steps, like for the variable stepsize integrators. I don’t think we bothered doing that in the tests of the variable stepsize integrators.
The order (assuming that there is some fixed convergence order) would still show up as the slope of the error-work graph, see, e.g., #4194 (comment).
There was a problem hiding this comment.
I haven't found any information on the convergence order either. I have found remarks that the iteration converges geometrically, but that isn't really relevant.
If we see a fixed convergence order in the error-work graph we can always add a test.
Co-authored-by: Robin Leroy <egg.robin.leroy@gmail.com>
This change adds an
Integratorsubclass which implements Чебышёв-Picard iteration.Чебышёв-Picard iteration (a combination of Чебышёв approximation with Picard iteration) was first proposed by Clenshaw and Norton in 1963. It has since been studied by Shaver, Feagin and Narcozy, Fukushima, and recently by Junkins et al (see also this NASA report from 1969 which provides an implementation in FORTRAN).
Given a first order ordinary differential equation$x'=f(x,t)$ with initial condition $x(t_0)=x_0$ , it can be used to solve the ODE on an interval $[t_0, t_f]$ :
See the linked papers for more details.
There is evidence that, together with some of the optimizations below, it has better performance than… some other integrators which Principia does not use.
Regarding the implementation
The linear algebra code uses the formulation (and notation) from Macomber's thesis.
I made the integrator a subclass of
FixedStepSizeIntegratorbecause it has a size of something like a step which is fixed. I implementedWriteToMessagebut notReadFromMessageas I wasn't sure of the best way to return a const reference while avoiding a memory leak.Regarding future work
The following is yet to be implemented: