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

Prevent (most) NaN propagation in Background and BoxcarExtract #159

Merged
merged 3 commits into from
Dec 21, 2022

Conversation

ojustino
Copy link
Contributor

@ojustino ojustino commented Dec 14, 2022

This pull request should be part 1 of a multi-pronged response to #158. Tests are forthcoming.

Background

Goal: NaNs outside the user-specified window should not propagate into the 1D spectrum of the background or the background-subtracted image. NaNs inside the user-specified window should be ignored.

Approach:

  • Adjusted _bkg_array:
    • Take the user-provided mask (if it exists) into account by bringing in the or_mask approach from HorneExtract that also masks non-finite pixels.
    • Fixed a possible mistake in the statistic='median' case that may have previously allowed zero-weighted pixels to still make it into the median calculation.
  • bkg_image() and sub_image() don't appear to need changes as long as _bkg_array handles NaNs appropriately.
  • bkg_spectrum() and sub_spectrum() just need np.sum changed to np.nansum.

BoxcarExtract

Goal: As with Background, NaNs outside the user-specified window should not propagate into the extracted 1D spectrum. Unlike with Background, NaNs inside the user-specified window should propagate to the extracted 1D spectrum. Zeroing them out of the sum could lure less careful users into finding spectral features that don't actually exist.

Approach:

  • Assign no weight to non-finite pixels while summing each column. This case needs to be handled specially because np.nan * 0 = np.nan, not 0. _parse_image() can't help because, for now, it only either adopts the user's mask or, if none is provided, masks every non-finite pixel. It can't only mask non-finite pixels outside the window.
    • The better approach going forward may be for _parse_image() to always defer to the user's mask, even if none is provided. Making our operations more resistant to unmasked NaNs we may see as a result would require major changes in another ticket.

@ojustino ojustino force-pushed the background-handle-nans branch from 66f38a6 to 994e338 Compare December 14, 2022 20:07
@ojustino ojustino force-pushed the background-handle-nans branch from 994e338 to 1edc7b6 Compare December 16, 2022 21:56
@codecov
Copy link

codecov bot commented Dec 16, 2022

Codecov Report

Merging #159 (bd72fae) into main (7be9a76) will increase coverage by 0.80%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
+ Coverage   75.77%   76.58%   +0.80%     
==========================================
  Files           9        9              
  Lines         706      709       +3     
==========================================
+ Hits          535      543       +8     
+ Misses        171      166       -5     
Impacted Files Coverage Δ
specreduce/background.py 93.75% <90.00%> (+5.45%) ⬆️
specreduce/extract.py 95.09% <100.00%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@kecnry kecnry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acts as I'd expect in the cases that used to result in a column of nans in the background image, and the implementation seems reasonable and clean to me! Happy to re-review/approve once test coverage is added and passes. Thanks!

Copy link
Contributor

@tepickering tepickering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, too. thanks for this! approved pending the added test coverage codecov is requesting.

@ojustino ojustino force-pushed the background-handle-nans branch from 9927182 to bd72fae Compare December 21, 2022 15:53
@ojustino ojustino merged commit 7043f78 into astropy:main Dec 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants