-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding stage-specific MetaClass objects and organizing default ECF values #632
base: main
Are you sure you want to change the base?
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Format looks fine, please continue.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting idea - I'll look into it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
S1MetaClass
which is a child of the originalreadECF.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 ofsuperbias_file
ifcustom_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 ofgetattr
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-specificMetaClass
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.