kevin218 / Eureka

Eureka! is a data reduction and analysis pipeline intended for time-series observations with JWST.
https://eurekadocs.readthedocs.io/
MIT License
60 stars 47 forks source link

POET transit and eclipse models, SPAM LD #609

Closed kevin218 closed 7 months ago

kevin218 commented 8 months ago

This PR adds an alternate option to BATMAN when fitting planet transits and eclipses. This also allows users to fit two planet transits, one with BATMAN and the other with POET. The sinusoidal phase curve model also depends on using BATMAN, so I've ported over the POET phase curve model.

Tests show consistent results between BATMAN and POET LC fits. In most cases, they are similar in speed; however, POET sees a ~50x improvement in speed using when using the 4-parameter LD model because it adopts the small-planet approximation (which is valid for RpRs < 0.1).

Because I need it for the same project, I've added the ability at Stage 4 to load in limb-darkening files (columns are wavelength, u1, u2, etc.) generated by SPAM. The file can be an arbitrarily high resolution and the new code will compute a simple average for each spectroscopic channel.

What's left to do...

What I'm leaving for another day...

codecov-commenter commented 8 months ago

Codecov Report

Attention: Patch coverage is 71.62954% with 242 lines in your changes are missing coverage. Please review.

Project coverage is 58.83%. Comparing base (0a53ad7) to head (06b1483). Report is 5 commits behind head on main.

Files Patch % Lines
.../eureka/S5_lightcurve_fitting/models/PoetModels.py 80.12% 64 Missing :warning:
...a/S5_lightcurve_fitting/models/DampedOscillator.py 18.60% 35 Missing :warning:
src/eureka/S4_generate_lightcurves/generate_LD.py 3.84% 25 Missing :warning:
src/eureka/S5_lightcurve_fitting/s5_fit.py 67.27% 18 Missing :warning:
src/eureka/S6_planet_spectra/s6_spectra.py 39.28% 17 Missing :warning:
...ka/S5_lightcurve_fitting/models/LorentzianModel.py 75.00% 16 Missing :warning:
src/eureka/S4_generate_lightcurves/s4_genLC.py 13.33% 13 Missing :warning:
...ureka/S5_lightcurve_fitting/models/BatmanModels.py 92.10% 12 Missing :warning:
...S5_lightcurve_fitting/models/SinusoidPhaseCurve.py 78.57% 9 Missing :warning:
src/eureka/S5_lightcurve_fitting/plots_s5.py 72.41% 8 Missing :warning:
... and 8 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #609 +/- ## ========================================== + Coverage 57.49% 58.83% +1.33% ========================================== Files 98 101 +3 Lines 12002 12636 +634 ========================================== + Hits 6901 7434 +533 - Misses 5101 5202 +101 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

taylorbell57 commented 8 months ago

It would seem much cleaner to me to generalize the transit fitting code to allow multiple planets rather than requiring a different type of transit model for each transit. I have something in trappist1_phasecurves that does this in a very hacky way for the TRAPPIST-1 b+c phase curve (which has transits of b, c, and g; eclipses of b and c; and phase variations of b and c), but that code is currently very hard-coded for that system and not generalized at all (and only works for the starry code). That's not to say that adding the POET transit model isn't worthwhile (especially given the speed improvement for the 4-parameter LD law), just that doing so for the purpose of fitting two transits doesn't seem like a good tactic to me (as we're going to need to support >2 planets anyway). Additionally, it will likely be confusing to users to have the names of the transit model parameters vary between the batman and the POET implementations; if someone wants to switch their model, they'll have to rename all of the variables in their EPFs which would be annoying. And folks writing an EPF from scratch might use some weird mixture of both parameter sets by accident.

Additionally, rather than adding a different phase curve model for the POET transit, I would suggest generalizing the current phase curve model to work with either the batman or the POET transit/eclipse models; the two phase curve models are mathematically the same model just coded in two different ways, so it'll save us a fair bit of duplicated code and some maintenance effort if we just merge the two functions.

kevin218 commented 8 months ago

I couldn't find a reasonable way to implement two-planet fits with BATMAN, but when I get back from vacation I'll take a look at your branch to see what you did.

I still want to implement POET transit and eclipse models. If we can get two-planet fitting up and running quickly, then I'm happy to adopt the same parameter names between POET and BATMAN. I'm on a bit of a time crunch to solve this problem because we're also working on a dataset with multiple planet transits.

I'll take a second look at broadening the phase curve model to allow for POET and BATMAN models. That was the major concern I had.

kevin218 commented 7 months ago

@taylorbell57 I think I've done more than enough damage in this PR. If we find a good way to merge phase curve models down the road, then we can decide to cross that bridge when we get to it. I've tested multi-transit and multi-eclipse fits with both POET and BATMAN. I didn't have a good multi-planet phase curve dataset to play with, but I know you do. If this PR works for T1-b+c then I'll be extremely happy.

taylorbell57 commented 7 months ago

Sorry, just added one other commit fixing an urgent bug where meta.compute_ltt was being ignored

taylorbell57 commented 7 months ago

Everything looks good to me and all the tests are passing which is great. I'll just try doing a quick optimization fit to the TRAPPIST-1 b+c phase curve data to ensure it runs properly and check if its generally consistent with the batman model

taylorbell57 commented 7 months ago

FYI, I've identified a couple of issues (some of which are related to this PR, some of which aren't). I'll try to separate and specify the issues related to this PR and may potentially just submit patches myself for several of them (likely as a PR that merges into this PR so you can look over the changes). One example of a current issue with this PR is that the alias between fp and fpfs is broken since neither fp nor fpfs are ever set to None. I've already locally tweaked the code to patch that bug and several others