Skip to content
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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

hpparvi
Copy link
Contributor

@hpparvi hpparvi commented Feb 27, 2025

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 in tox.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, and appy_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 suggested mask_extraction_bin and apply_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 or apply-mask-only? The first option is probably grammatically better, but the other would be easier to remember.

@hpparvi hpparvi added this to the v1.5 milestone Feb 27, 2025
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 93.33333% with 11 lines in your changes missing coverage. Please review.

Project coverage is 86.16%. Comparing base (c31a9af) to head (e33f10e).

Files with missing lines Patch % Lines
specreduce/extract.py 91.52% 5 Missing ⚠️
specreduce/tracing.py 89.18% 4 Missing ⚠️
specreduce/core.py 95.45% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@hpparvi
Copy link
Contributor Author

hpparvi commented Feb 27, 2025

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.

mask_treatment.pdf

@hpparvi
Copy link
Contributor Author

hpparvi commented Feb 27, 2025

Also, I still need to improve the tests for the new code.

@tepickering
Copy link
Contributor

Also, I still need to improve the tests for the new code.

shall i wait for you to do this before doing a review?

@hpparvi
Copy link
Contributor Author

hpparvi commented Feb 28, 2025

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.

hpparvi added 18 commits March 1, 2025 17:35
…", "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.
…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.
- Fixed BoxcarExtraction bug with mask_treatment=='apply'.
@hpparvi
Copy link
Contributor Author

hpparvi commented Mar 1, 2025

Okay, the PR is now ready for review, @tepickering .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants