Skip to content

Only apply nan mask workaround for cp version below 10.0.#2689

Merged
j9ac9k merged 3 commits intopyqtgraph:masterfrom
koenstrien:revert-outdated-cp-bugfix
Apr 14, 2023
Merged

Only apply nan mask workaround for cp version below 10.0.#2689
j9ac9k merged 3 commits intopyqtgraph:masterfrom
koenstrien:revert-outdated-cp-bugfix

Conversation

@koenstrien
Copy link
Copy Markdown
Contributor

In #1595 a workaround was introduced for masking NaNs in cupy arrays, because the indexing is different than for numpy arrays. As of cupy version 10.0.0 the indexing can be done the same for numpy and cupy arrays again (see cupy PR #6196) and the workaround causes issues. This is solved by only applying the workaround if the cupy version is lower then 10.0.0. In the future the workaround can be moved completely.

Fixes #1595

@koenstrien
Copy link
Copy Markdown
Contributor Author

Right now distutils.version.StrictVersion is used for the version check. Maybe packaging.version is better (vendored by setuptools too), but I think this would mean packaging should be added to the requirements.

@pijyoi
Copy link
Copy Markdown
Contributor

pijyoi commented Apr 14, 2023

Several places in the library do something like:

tuple(map(int, cp.__version__.split("."))) < (10, 0)

@koenstrien
Copy link
Copy Markdown
Contributor Author

That is another possibility if preferred.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Apr 14, 2023

I would avoid using distutils as it is being removed in Python 3.12. In testing code paths, you can use the packaging library as that is a dependency of pytest but for runtime code, we should probably stick to what @pijyoi suggested above (although I'm certainly open to other suggestions).

@koenstrien
Copy link
Copy Markdown
Contributor Author

I have updated the implementation.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Apr 14, 2023

Hi @koenstrien

I hope you don't mind, I pushed a commit to your branch removing the import. The reason I did it is I'm about to leave for a vacation for a little over a week, and I wanted to do a new release before I left. I wanted this PR to be in the new release which I'm about to get the ball rolling on.

Thanks for the PR!

EDIT: whops, I just realized that I never actually submitted the notification to you asking to remove the import, my fault! (I was on mobile, meant to leave a comment on a line, guess it started a review, and when I hit done, it thought I was done w/ the comment but not the review).

@j9ac9k j9ac9k merged commit 33f8da2 into pyqtgraph:master Apr 14, 2023
@koenstrien koenstrien deleted the revert-outdated-cp-bugfix branch April 17, 2023 09:09
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