-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove npcompat.moveaxis #5345
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Remove npcompat.moveaxis #5345
Conversation
Hello @Illviljan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-05-20 20:31:33 UTC |
It's probably worth double-checking that the more complex Variable |
I don't understand how this works, @max-sixty. You're adding the variable The numpy implementation seems very similar to the npcompat version: Maybe the npcompat code was copy/pasted just to avoid waiting on numpy releases? |
Yes, pytest can be a bit confusing at first, but it's a really nice framework — I agree this seems like it should be fine, and maybe I'm being a bit too cautious. @keewis could you give this a check given my track-record is blotchy now, and we can merge if you agree? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does look good to me, but I'm somewhat confused about one of the comments that were removed.
# Vendored from NumPy 1.12; we need a version that support duck typing, even | ||
# on dask arrays with __array_function__ enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shoyer if you get a moment, could you take a glance at this and see if it looks OK, or we should add more tests? |
Thanks @Illviljan ! |
Remove some compatibility code that doesn't seem necessary anymore.
pre-commit run --all-files