Adding stage-specific MetaClass objects and organizing default ECF values #632
Conversation
These new MetaClass children hold 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. This new addition will also allow for cleaner code with fewer instances of `if hasattr(meta, ...)`.
I found a cleaner way of doing things that doesn't require any additional inputs from the user.
taylorbell57
added
enhancement
| if self.dq_sat_mode == 'percentile': | ||
| self.dq_sat_percentile = getattr(self, 'dq_sat_percentile') # Force this to be specified if dq_sat_mode is 'defined'. The percentile of the entire time series to use to define the saturation mask (50=median) | ||
| elif self.dq_sat_mode == 'defined': | ||
| self.dq_sat_columns = getattr(self, 'dq_sat_columns') # Force this to be specified if dq_sat_mode is 'defined' |
There was a problem hiding this comment.
Should we consider the case here if dq_sat_mode isn't any of the given options? This question applies more broadly to other meta values. It might be cleaner to catch incorrect settings in the s#_meta.py files than in the code.
There was a problem hiding this comment.
Hmm, interesting idea - I'll look into it
There was a problem hiding this comment.
We need to be a bit careful in how extensively we check for incorrect settings though since that might make these s#_meta.py files enormous.
| self.grouplevel_bg = getattr(self, 'grouplevel_bg', False) | ||
| if self.grouplevel_bg: | ||
| self.ncpu = getattr(self, 'ncpu', 4) | ||
| self.bg_y1 = getattr(self, 'bg_y1') # Force this to be specified if grouplevel_bg is True | ||
| self.bg_y2 = getattr(self, 'bg_y2') # Force this to be specified if grouplevel_bg is True | ||
| self.bg_deg = getattr(self, 'bg_deg', 0) | ||
| self.bg_method = getattr(self, 'bg_method', 'mean') # Options: std (Standard Deviation), median (Median Absolute Deviation), mean (Mean Absolute Deviation) | ||
| self.p3thresh = getattr(self, 'p3thresh', 5) | ||
| self.bg_disp = getattr(self, 'bg_disp', False) # Row-by-row BG subtraction (only useful for NIRCam) | ||
| self.bg_x1 = getattr(self, 'bg_x1', None) # Left edge of exclusion region for row-by-row BG subtraction | ||
| self.bg_x2 = getattr(self, 'bg_x2', None) # Right edge of exclusion region for row-by-row BG subtraction |
There was a problem hiding this comment.
Now would be a good time to consider renaming some variables that aren't particularly descriptive. This comment applies to all stages.
There was a problem hiding this comment.
Yeah, fair point... Implementing that will be pretty easy with VS Code, but choosing which parameters to rename and then choosing better names will be the more time-intensive part. Maybe I'll do a first pass of moving things into stage-specific MetaClass objects like this so we can see all the variables in one place, and then we can go through and decide on some better names?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #632 +/- ##
==========================================
+ Coverage 54.01% 58.49% +4.48%
==========================================
Files 101 102 +1
Lines 12692 12720 +28
==========================================
+ Hits 6855 7440 +585
+ Misses 5837 5280 -557 ☔ View full report in Codecov by Sentry. |
Makes S4 exotic-ld automatic filter choices easier
This draft PR introduces the
S1MetaClasswhich is a child of the originalreadECF.MetaClassobject.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
AttributeErrorwill 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 ofsuperbias_fileifcustom_biasis 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 ofgetattrwithin 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-specificMetaClasswhich 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.