search

kevin218 / Eureka

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

Alternate Stage 3 Inputs #603

Closed AarynnCarter closed 1 month ago

AarynnCarter commented 6 months ago

Pull request to allow for non-Eureka! inputs to Stage 4.

Leaving for a future PR:

This PR resolves part of #404

codecov-commenter commented 6 months ago

Codecov Report

Attention: Patch coverage is 69.04762% with 26 lines in your changes missing coverage. Please review.

Project coverage is 54.28%. Comparing base (a344dbe) to head (ab7a32a). Report is 3 commits behind head on main.

Files Patch % Lines
src/eureka/S4_generate_lightcurves/s4_genLC.py 23.80% 16 Missing :warning:
src/eureka/lib/manageevent.py 77.77% 8 Missing :warning:
src/eureka/S3_data_reduction/s3_reduce.py 75.00% 1 Missing :warning:
src/eureka/lib/util.py 95.45% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #603 +/- ## ========================================== - Coverage 54.29% 54.28% -0.01% ========================================== Files 101 101 Lines 12713 12779 +66 ========================================== + Hits 6902 6937 +35 - Misses 5811 5842 +31 ```

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

AarynnCarter commented 2 months ago

@kevin218 , @taylorbell57 Okay I made some tweaks based on the comments. Sorry if I couldn't get absolutely everything done, I think we at least have a good foundation to build from for more specific custom data inputs and / or changes to the tutorial. I don't think I'll have a chance to put more work into this, so if there's any more tweaks you'd like to make, it would probably be best to checkout my branch and incorporate them directly - I'll be sure to leave it as it currently is! 😄

kevin218 commented 2 months ago

@taylorbell57 Do you also want to review changes to this PR before merging into main?

taylorbell57 commented 2 months ago

@taylorbell57 Do you also want to review changes to this PR before merging into main?

Yes please, I'll likely do so this weekend or on Monday

taylorbell57 commented 1 month ago

I know you're busy right now @AarynnCarter, so I'll try to address my own comments in the next week or so

AarynnCarter commented 1 month ago

Thanks @taylorbell57, sorry to drop the PR on you not quite finished!

taylorbell57 commented 1 month ago

Alright, I've fixed most of the known issues with the PR. I still need to review the new tutorial and potentially make some changes to the S4 checks (depending on whether this PR gets merged after or before PR #632).

taylorbell57 commented 1 month ago

@AarynnCarter, I fixed some very minor typos in your excellent tutorial, but there was also an error in how you setup the photometry SpecData.h5 file. To be sure that Stage 4 functions properly, the wave_1d array must still be defined, otherwise integrations with ExoTiC-LD and some of the Eureka! code is going to break.

I believe my implemented change should work well, but I need you to re-run the notebook to update all the notebook outputs; hopefully this should take only ~1 minute of your time. It's possible that I should've done wave_1d = np.array([2.1,]) instead of wave_1d = 2.1 - if you get an error message, then try adjusting the code to that.

AarynnCarter commented 1 month ago

@taylorbell57 No worries, I'll aim to update things by the end of next week!

AarynnCarter commented 1 month ago

@taylorbell57 Okay, all done! You were right about it needing to be cast as a numpy array. There was also a typo I fixed in the allocation of the x value.