-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
Mask and non-finite option refactor #254
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
+ Coverage 85.93% 86.16% +0.22%
==========================================
Files 13 13
Lines 1166 1178 +12
==========================================
+ Hits 1002 1015 +13
+ Misses 164 163 -1 ☔ View full report in Codecov by Sentry. |
Looks like I still need to fix some minor issues with the docs. Here is a print of the doc page illustrating the mask treatment options. |
Also, I still need to improve the tests for the new code. |
shall i wait for you to do this before doing a review? |
Yes, please. I should have the last issues fixed today. I'll let you know when this is ready. |
…", "zero-fill", "nan-fill", "apply_mask_only", and "apply_nan_only".
…zero-fill`, `nan-fill`) and simplified parameter documentation.
… change ensures compatibility with PEP 8 while avoiding conflicts with black's formatting rules.
…ted the code with black.
…tion extends the mask across the entire cross-dispersion axis when a masked or non-finite pixel is encountered.
…s valid mask treatment options, updating the documentation and test cases accordingly. Modified the tests to validate the new options and ensured compatibility with existing functionality.
… extraction tests.
- Fixed BoxcarExtraction bug with mask_treatment=='apply'.
Okay, the PR is now ready for review, @tepickering . |
This PR addresses Issue #252 by renaming the existing mask treatment options and introducing a new set. It also adds a documentation page with illustrations demonstrating each option's effect in practice, and it also adds a basic Black configuration to
pyproject.toml
and modifies flake8 intox.ini
to ignore E203, as described here. Finally, I started running black on the code I edited, which leads to somewhat larger diffs than from the actual functional changes alone.The complete mask treatment option set now includes:
apply
,ignore
,propagate
,zero-fill
,nan-fill
,apply_mask_only
, andappy_nan_only
. Now is the best time to let everyone know if you are not happy with some of these choices, so please write if you have strong feelings against any :)I chose
propagate
over the suggestedmask_extraction_bin
andapply_to_full_extraction_bin
because it is more generic and can be used in any method or function requiring this behavior, rather than being limited to spectrum extraction. However, I'm happy to change this to something else if anyone can suggest a good, clear alternative.Finally, should we go for
apply_mask_only
orapply-mask-only
? The first option is probably grammatically better, but the other would be easier to remember.