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

Remove JLD dependency #12

Merged
merged 46 commits into from
Jun 7, 2023
Merged

Remove JLD dependency #12

merged 46 commits into from
Jun 7, 2023

Conversation

PerezHz
Copy link
Owner

@PerezHz PerezHz commented May 28, 2023

After merging of PerezHz/PlanetaryEphemeris.jl#17 it's now possible in principle to remove the JLD dependency; this PR is WIP directed towards that goal. Of course, for things to be consistent we will need our Solar System ephemeris to also include our integration of TT-TDB.

@LuEdRaMo, I reckon you're working in uploading a new 100 years ephemeris including TT-TDB; could you please drop me a line here when the ephemeris are ready?

@LuEdRaMo
Copy link
Collaborator

LuEdRaMo commented May 28, 2023

@LuEdRaMo, I reckon you're working in uploading a new 100 years ephemeris including TT-TDB; could you please drop me a line here when the ephemeris are ready?

That's correct, the integration just finished and there are a couple points worth mentioning:

  • The 100 years integration took ∼19 hrs on 10 threads.
  • The new file size is ∼556 MB.
  • I've plotted the difference in TT-TDB between PE and JPL, it seems to be lower than $10^{-7}$ seconds by 2100.
  • To continue with this PR I need to upload the new ephemeris file to Github, give me a couple of minutes.

ttmtdb_PE_vs_JPL

@lbenet
Copy link
Collaborator

lbenet commented May 28, 2023

Nice performance and memory usage improvements!

Regarding the plot, which I think altogether displays an overall good precision, I find surprising the huge precision loss in the first steps of the integration. Any ideas why does this occur? Do you observe the same behavior for the integration backwards in time?

@LuEdRaMo
Copy link
Collaborator

Nice performance and memory usage improvements!

Regarding the plot, which I think altogether displays an overall good precision, I find surprising the huge precision loss in the first steps of the integration. Any ideas why does this occur? Do you observe the same behavior for the integration backwards in time?

  • I don't know what could be the reason for such precision loss. Could it be a term in the corresponding equations @PerezHz ?
  • I will do an integration 100 years backward, but that will take another 19 hrs.
  • By the way, the new ephemeris file is already on Github. It can be found in the same URL that is in the artifacts. I don't know if it is necessary to regenerate the Artifacts.toml.

@PerezHz
Copy link
Owner Author

PerezHz commented May 28, 2023

Thank you @LuEdRaMo for updating the ephemeris and uploading them to GitHub! Also thanks for the plot, that's really helpful!

Regarding the plot, which I think altogether displays an overall good precision, I find surprising the huge precision loss in the first steps of the integration. Any ideas why does this occur?

That's certainly a good point; I wonder if we plot the relative error, i.e., abs((x_PE - x_JPL)/x_PE) (where by x I mean TT-TDB), as a function of time, we would see something similar? My point is that the TT-TDB difference grows over time, so perhaps part of what we're seeing is the growth in time of TT-TDB itself.

Anyway, I would say one major contribution to the differences is the fact that our integration of the asteroids (whose contributions to TT-TDB are considered in the integration) is different from that of JPL (we have errors ~km or even more in asteroid positions over ~50 years). So at the start the error grows a lot due to the different integrations, but then the error stabilizes, since the difference in position cannot be larger than the size of the asteroid belt, maybe? One way to check this would be to plot the errors in (at least some of the) asteroid positions and see if they display similar behavior.

EDIT/UPDATE: we later found that the largest contribution to the behavior of the above plot is a race condition in the integration of TT-TDB, for further details see: PerezHz/PlanetaryEphemeris.jl#21 (comment)

@PerezHz
Copy link
Owner Author

PerezHz commented May 28, 2023

I don't know what could be the reason for such precision loss. Could it be a term in the corresponding equations @PerezHz?

That can certainly be the case; what I'll do next is to see how the Apophis results change with the new ephemeris, since radar data, and hence the fit, is really sensitive to any mismodeling. That can also show that we have successfully removed the JLD dependency. In the end, if the results are not modified to a significant level, then I would say it fits our current purposes here in NEOs.

By the way, the new ephemeris file is already on Github. It can be found in the same URL that is in the artifacts. I don't know if it is necessary to regenerate the Artifacts.toml.

Many thanks @LuEdRaMo! I've already updated the generate_artifacts.jl script and Artifacts.toml to remove the TT-TDB related artifacts, but probably I'll do a bit more house-keeping around there.

@LuEdRaMo
Copy link
Collaborator

I noted we have StaticArrays as a dependency, but the only object we use is SArray in src/observations/topocentric.jl. If we don't need that package for anything else, we could change to StaticArraysCore which contains only the basic static types (like SArray).

@lbenet
Copy link
Collaborator

lbenet commented May 29, 2023

I noted we have StaticArrays as a dependency, but the only object we use is SArray in src/observations/topocentric.jl. If we don't need that package for anything else, we could change to StaticArraysCore which contains only the basic static types (like SArray).

Good catch! I agree with you. Have you tested it?

@PerezHz
Copy link
Owner Author

PerezHz commented May 29, 2023

Just wanted to clarify my last answer to @LuEdRaMo's question: we do are including all the relevant terms to integrate the ephemeris, except those ones related to specific observing stations; those terms are only important to fit the dynamical model to interferometry (VLBI) observations where precision to nanosecond level becomes important. Otherwise, so far as I've been able to check, the expressions we are using are correct, and I think that is reflected in the error being ~30nanoseconds in the 100 years integration. But again, that's why checking the results here for Apophis makes sense, since an error in TT-TDB will probably show up here.

@PerezHz
Copy link
Owner Author

PerezHz commented May 29, 2023

Quick update: I did a bit of house-keeping in the artifact generation department, so that the dev/generate_artifacts.jlscript now works for all artifacts on julia 1.9. Also, I redirected the JPL artifacts from the PerezHz/jpleph repo to the JPL FTP server directly, which saves us from having to maintain exact copies of the same files. More importantly, we are now free (in principle) of the JPL DE430 ephemeris for TT-TDB saved as a TaylorInterpolant in JLD format, so that's one less thing to worry about. Rather, now we have to worry about getting our own TT-TDB integration right 😅 .

Regarding SArray, I agree with @LuEdRaMo's take that it makes sense to only load StaticArraysCore instead of the full thing. I can try to make that change here and then see how the orbit determination pipeline reacts.

@LuEdRaMo
Copy link
Collaborator

I'am trying to use CairoMakie to plot results from NEOs. However, CairoMakie (NEOs) requires StatsBase v0.33 (v0.34). Since StatsBase is not directly used within NEOs, I think it is safe to change the compat entry to

StatsBase = "0.33, 0.34"

@PerezHz PerezHz changed the title WIP: remove JLD dependency Remove JLD dependency Jun 4, 2023
@PerezHz
Copy link
Owner Author

PerezHz commented Jun 4, 2023

With the latest changes, and pending confirmation from @LuEdRaMo that I haven't broken anything too bad, this PR is in principle ready for review. I would like to note that JLD is still mentioned in some notebooks (which we should consider updating), but in source code as such, I think the hardest dependency was the TT-TDB ephemeris from JPL; otherwise we only used JLD to save files. Things still pending is adding tests for orbit propagation with jet transport, and orbit determination, but we can tackle those tasks in another PR, since this has already grown a bit beyond what I was expecting at first...

@PerezHz
Copy link
Owner Author

PerezHz commented Jun 4, 2023

I think the hardest dependency was the TT-TDB ephemeris from JPL

By that I mean that before we depended on JLD.jl (and the JPL ephemeris) to get TT-TDB

@LuEdRaMo
Copy link
Collaborator

LuEdRaMo commented Jun 6, 2023

I'll do a few test integrations to check everyting works fine...

All integrations I have done so far have worked perfectly.

@PerezHz
Copy link
Owner Author

PerezHz commented Jun 6, 2023

All integrations I have done so far have worked perfectly.

Awesome! Many thanks for checking! I'm trying to bring radec_astrometry and radar_astrometry to some kind of "common" interface, which we are already doing by overloading residuals, and part of that involves turning the number of iterations in the light-time solution, niter, into a keyword argument. The default value is most of the time used anyway, and if the non-default is used, I think it's clearer to then have, say niter=12, or what have you. This is already done for radar astrometry, but my worry is that this may break downstream computations that probably you're working on right now with optical astrometry, so I just wanted to give a heads-up and mention it before pushing... what do you think, @LuEdRaMo?

@LuEdRaMo
Copy link
Collaborator

LuEdRaMo commented Jun 6, 2023

So far I've been using the default value of niter, so as long as the first argument (the observations vector) remains the same, the changes you mention shouldn't have a major impact on my current calculantions.

@PerezHz
Copy link
Owner Author

PerezHz commented Jun 6, 2023

So far I've been using the default value of niter, so as long as the first argument (the observations vector) remains the same, the changes you mention shouldn't have a major impact on my current calculantions.

Alright! Then this change should be transparent for you 👍

@PerezHz
Copy link
Owner Author

PerezHz commented Jun 6, 2023

Okay, I've made a few last changes. Besides passing niteras a kwarg, I'm also now proposing to avoid throughout the code the use of ::Function, which can help triggering compilation of specialized methods. I updated docstrings as much as I could but probably there's some stuff that I might've missed; if anything needs updating please let me know; otherwise we can always update those later. I would propose to bump a minor version, merge this, and then focus on #13, what do you guys think, @lbenet @LuEdRaMo?

@PerezHz
Copy link
Owner Author

PerezHz commented Jun 7, 2023

Tests are passing so I'll go ahead and merge.

@PerezHz PerezHz merged commit dd5ee05 into main Jun 7, 2023
@PerezHz PerezHz mentioned this pull request Jun 7, 2023
@PerezHz PerezHz deleted the jp/jld branch June 7, 2023 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants