Repository navigation
Acceleration fit - #450
Acceleration fit#450rt538-del wants to merge 10 commits into
Conversation
Add Basis.set_acceleration_priors (acc_ra, acc_dec) and System options acceleration_only and acceleration_and_keplerian_fit, with the acceleration model, setup, and save support in system.py.
|
Unit tests pass on 3.10/3.12/3.14. The notebook job fails on Eps_Eri_Joint_Fit (data file path) and Gaia_DR4_EAS (3000 s timeout). Neither notebook uses the new acceleration code. The only change to existing code in plot_dr4_orbit is behind the new remove_acceleration=False option. |
| use_rebound=False, | ||
| acceleration_only=False, | ||
| acceleration_and_keplerian_fit=False, | ||
| alpha0=None, |
There was a problem hiding this comment.
Rather than passing these (alpha0, delta0, alphadec0_epoch) into System, can you access them directly from the Gaia object?
| X, Y, Z = predictor.X, predictor.Y, predictor.Z | ||
| else: | ||
| bary_pos, _ = get_body_barycentric_posvel( | ||
| "earth", Time(epochs, format="decimalyear") |
There was a problem hiding this comment.
make sure to use julian year instead of decimalyear throughout
There was a problem hiding this comment.
Say you wrote the tutorial at the top!
There was a problem hiding this comment.
Feel free to not address this if you don't have time, but I think it would be very interesting to do the long-period fit using an acceleration parametrization and with a full keplerian parametrization to see how different the posterior predicted accelerations are.
There was a problem hiding this comment.
Never mind, I got to the later part of the tutorial and saw that you're fitting the same dataset with both a keplerian and an acceleration term (which is super interesting-- I'll be excited to see those results once they converge)
There was a problem hiding this comment.
In the "Likelihood object" cell, you write plot._model_along_scan_dr4 returns w_i for one parameter vector-- presumably you really want to reference gaia.compute_lnlike or some such?
There was a problem hiding this comment.
This is mentioned a few places throughout the tutorial
There was a problem hiding this comment.
stellar_or_system_mass and plx are placeholders that aren't used -- we should really make this syntax better for users. What if we create plx_prior and stellar_or_system_mass_prior kwargs in System that overwrite plx, stellar_or_system_mass and their associated errors if they are passed in? That will allow us to maintain backwards compatibility but make it easier for users to set priors on these params.
There was a problem hiding this comment.
In "Starting point," follow Clarissa's updated tutorial which uses an orbitize function for performing the linear fit and choosing the starting point for the model.
There was a problem hiding this comment.
Same for Keplerian + acceleration starting point
There was a problem hiding this comment.
What notebook.. sorry I couldn't find it
There was a problem hiding this comment.
np.hypot! I've never seen that function before. Very useful, thanks for teaching me.
There was a problem hiding this comment.
We should make it so that the Delta t^2 coefficient is the second derivative of the motion, not half the second derivative of the motion.
There was a problem hiding this comment.
add default axis labels for acc_ra/acc_dec
There was a problem hiding this comment.
in the along-scan data and residuals, can you make the model smoother (i.e. plot at a greater number of epochs, not just the observation epochs)
There was a problem hiding this comment.
Nvm, realized afterward that's nonsensical. I liked the way you visualized it lower down in the notebook with the point-to-point comparison rather than the jagged line.
There was a problem hiding this comment.
The notebook takes quite a long time to run (which is fine for actually performing the fit, but the version that needs to run in real time when we do continuous integration testing should be faster). Can you run for fewer steps, but leave the number of steps you would need for convergence in there commented out?
There was a problem hiding this comment.
"The OHP files only give the along-scan parallax factor
--> you can reconstruct the parallax factors given the scan angle, so it would be nice to add in the parallactic motion too.
There was a problem hiding this comment.
Big picture thoughts about plots (ccing @clarissardoo):
My "subtract each component" plot doesn't look great. Can you work on the aesthetics? Some ideas:
- bigger fonts by default
- thicker error bars
- label each y axis (i.e. communicate to the viewer what is actually being plotted more clearly)
- think about the color scheme. Can you create more contrast? Make sure the thick vs thin error bars are more visible.
- By default, this should also work for the kepler-only or kepler+accel fits.
Read star position and reference epoch from the Gaia object. Use Julian years. Define acc_ra and acc_dec as the second derivative of the motion. Add plx_prior and stellar_or_system_mass_prior to System. Add DR4LogProb.linear_fit for choosing starting points. Add default corner labels for the accelerations. Improve plot_dr4_acceleration, add parallax to it, and support Keplerian-only fits. Update the tutorial and tests.
These are changes made into the system.py and basis.py so the users are able to choose to whether they would like to do a acceleration only fit or a aceeleration+keplerian fit. This is related to issue #435