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

Correct default mask for array images in HorneExtract #118

Merged
merged 4 commits into from
Aug 18, 2022

Conversation

ojustino
Copy link
Contributor

When HorneExtract/OptimalExtract is given a numpy array as an image and no mask, it tries to create its own mask using np.ma.masked_invalid(image). They are currently written as if that object itself is the mask, when it's actually in that object's mask attribute. This pull request makes the switch.

@ojustino ojustino changed the title Get mask object from numpy masked_invalid Correct default mask for array images in HorneExtract Aug 15, 2022
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.

Fixes the case that I was seeing! Does this need a changelog entry as a bugfix?

@ojustino
Copy link
Contributor Author

Good call, @kecnry; I went ahead and created a new section in CHANGES.rst.

@ojustino ojustino requested a review from tepickering August 15, 2022 18:51
@codecov
Copy link

codecov bot commented Aug 15, 2022

Codecov Report

Merging #118 (e295641) into main (37d4502) will increase coverage by 0.66%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   69.43%   70.09%   +0.66%     
==========================================
  Files           9        9              
  Lines         602      602              
==========================================
+ Hits          418      422       +4     
+ Misses        184      180       -4     
Impacted Files Coverage Δ
specreduce/extract.py 89.47% <100.00%> (+3.00%) ⬆️

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

@tepickering
Copy link
Contributor

looks good and another nice catch. codecov says and i agree it'd be worth adding a test case to cover this.

@ojustino
Copy link
Contributor Author

The coverage needed here likely clashes with #117 since image is one of the parameters whose location is changed there. That one is close to complete, though we may need to discuss how to handle a merge given that it breaks the API.

@kecnry
Copy link
Member

kecnry commented Aug 15, 2022

@ojustino - if you add a test here that covers the case that used to fail, I can update it if needed when rebasing (since I'm guessing this will get merged before that one).

@ojustino
Copy link
Contributor Author

OK, the tests are in.

@ojustino ojustino force-pushed the correct-default-mask branch from aad2df9 to ada5936 Compare August 18, 2022 18:04
@ojustino ojustino merged commit abb5725 into astropy:main Aug 18, 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