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

TST: Add dev numpy to devdeps, fix compat with numpy 1.24 #153

Closed
wants to merge 1 commit into from

Conversation

pllim
Copy link
Member

@pllim pllim commented Dec 2, 2022

@codecov
Copy link

codecov bot commented Dec 2, 2022

Codecov Report

Merging #153 (ca1dc16) into main (6b2ffb0) will decrease coverage by 0.45%.
The diff coverage is 50.00%.

❗ Current head ca1dc16 differs from pull request most recent head 58434f7. Consider uploading reports for the commit 58434f7 to get more accurate results

@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
- Coverage   75.77%   75.32%   -0.46%     
==========================================
  Files           9        9              
  Lines         706      697       -9     
==========================================
- Hits          535      525      -10     
- Misses        171      172       +1     
Impacted Files Coverage Δ
specreduce/core.py 73.17% <50.00%> (-1.19%) ⬇️
specreduce/extract.py 94.70% <50.00%> (-0.36%) ⬇️

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

@pllim
Copy link
Member Author

pllim commented Dec 2, 2022

Hmm. Maybe #151 isn't the correct solution after all.

@ojustino
Copy link
Contributor

ojustino commented Dec 2, 2022

Hi @pllim, after some local testing, I think the best solution would be to drop masked_where() entirely. In the place that causes the error you found, all we really need is the mask; the other attributes that come with a masked_array are superfluous. All they do to create the mask is call isfinite(), so we can just do the same here.

This usage of masked_where() appears a few places in specreduce and I believe they should all should be changed in the same effort. That could be this one, or if you like, I can try it in a different pull request.

@pllim
Copy link
Member Author

pllim commented Dec 2, 2022

If you want to change mask logic, it is up to you as I am not familiar with the algorithms here. I just did track down why numpy dev is giving error and it might or might not be upstream bug. I am going to ask numpy people (see numpy/numpy#22720).

specreduce/core.py Outdated Show resolved Hide resolved
BUG: Compat with numpy 1.24.0rc1
@pllim
Copy link
Member Author

pllim commented Dec 5, 2022

As for the coverage, it will fix itself when numpy is released and your coverage job picks it up. Obviously it is covered because tests fail with dev numpy.

@pllim
Copy link
Member Author

pllim commented Dec 5, 2022

Maybe we can merge by the end of the week if numpy makes it clear that they have no intention of backing out this behavior.

@pllim pllim marked this pull request as ready for review December 5, 2022 15:17
@pllim pllim changed the title TST: Add dev numpy to devdeps TST: Add dev numpy to devdeps, fix compat with numpy 1.24 Dec 5, 2022
@ojustino
Copy link
Contributor

ojustino commented Dec 5, 2022

@pllim I opened #155 to make the edits I talked about earlier since most of the places that trigger this issue don't actually need to use mask_invalid(). Do you have any objections to that approach?

@ojustino
Copy link
Contributor

ojustino commented Dec 5, 2022

The other PR should probably still integrate the changes you made to tox.ini in order to catch these errors more quickly in the future.

@pllim
Copy link
Member Author

pllim commented Dec 5, 2022

@ojustino , yes, please do cherry-pick the dev numpy job over. If #155 supersedes this PR, feel free to close this without merge. Thanks!

@ojustino ojustino closed this in #155 Dec 5, 2022
@pllim pllim deleted the dev-numpy branch December 5, 2022 18:56
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.

2 participants