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 43 forks source link

Adding stage-specific MetaClass objects and organizing default ECF values #632

Open taylorbell57 opened 4 months ago

taylorbell57 commented 4 months ago

This draft PR introduces the S1MetaClass which is a child of the original readECF.MetaClass object.

This new class holds Stage 1 specific defaults for different instruments. This will allow for cleaner ECFs where values can be left at their defaults without specifying them in the ECF. For cases where a value is must be specified because there is no safe default value, an AttributeError will be raised (if desired, I can catch those exceptions and re-raise them with an explicit reminder to set those variables in their ECF). I also only raise these exceptions when I must (e.g., only require the specification of superbias_file if custom_bias is True).

This new addition will also allow for cleaner code with fewer instances of if hasattr(meta, ...) scattered throughout the code. I make frequent use of getattr within this new class which allows attributes to be set to a default value if the value hasn't been previously set while minimizing the lines of code. As I continue on to additional stages, I will move any default values scattered throughout the code to their stage-specific MetaClass which will further tidy the code. This will also make maintenance and backwards compatibility easier as new ECF settings can just have their defaults specified in a single, stage-specific location.

I'd like @kevin218's feedback (just a quick "Format looks fine, please continue", or "I don't like this format, let's discuss") at this stage before I work through all six stages to check that you're happy with the overall format in which I'm writing these classes. I know I'm not flake8 compliant right now, I'll fix that before I mark the PR as ready for review.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 71.16883% with 222 lines in your changes missing coverage. Please review.

Project coverage is 54.95%. Comparing base (a939236) to head (a6b88ac).

Files Patch % Lines
src/eureka/S1_detector_processing/s1_meta.py 6.12% 92 Missing :warning:
src/eureka/S4_generate_lightcurves/s4_meta.py 69.31% 27 Missing :warning:
src/eureka/S3_data_reduction/s3_meta.py 85.71% 22 Missing :warning:
src/eureka/S1_detector_processing/s1_process.py 5.26% 18 Missing :warning:
src/eureka/S6_planet_spectra/s6_meta.py 76.92% 12 Missing :warning:
src/eureka/S2_calibrations/s2_calibrate.py 76.66% 7 Missing :warning:
src/eureka/S1_detector_processing/ramp_fitting.py 0.00% 6 Missing :warning:
src/eureka/S5_lightcurve_fitting/s5_meta.py 91.04% 6 Missing :warning:
src/eureka/S1_detector_processing/group_level.py 0.00% 5 Missing :warning:
src/eureka/S1_detector_processing/bias_sub.py 0.00% 4 Missing :warning:
... and 13 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #632 +/- ## ========================================== + Coverage 54.24% 54.95% +0.71% ========================================== Files 101 107 +6 Lines 12793 13170 +377 ========================================== + Hits 6939 7237 +298 - Misses 5854 5933 +79 ```

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

taylorbell57 commented 2 weeks ago

I'm going to approve and merge Aarynn's PR now, so I'm reminding myself here that I will need to move the non-Eureka defaults from the following lines https://github.com/kevin218/Eureka/blob/ab7a32aaa2b0f18221fa42e478e1f1c19997b8b1/src/eureka/S4_generate_lightcurves/s4_genLC.py#L107-L139 to the S4MetaClass

taylorbell57 commented 2 weeks ago

Alright, that tweak after Aarynn's PR is now taken care of and this PR is ready for review again

kevin218 commented 4 days ago

As a general comment, I think we can pare down the S1 and S2 templates by removing many of the skip_pipeline_step keywords that never change.