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

Support testing np and tnp on the same test #77

Merged
merged 1 commit into from
Mar 4, 2023

Conversation

honno
Copy link
Contributor

@honno honno commented Mar 2, 2023

@ev-br here's how one could test both NumPy-proper and torch_np on the same test. Essentially any test case just needs a parameter np, which should be treated as the array module we want to test (i.e. numpy, torch_np), and we need to ensure we're using that module rather than outside imports.

I reworked some of test_indexing.py to demonstrate how this could be utilised. It's a bit finnicky as you have to check np is {numpy,torch_np} for some module-specific behaviour, e.g. raised error classes and skips/xfails, and you have to make sure you're using the argument np for things like the {numpy,torch_np}.testing utils.

Maybe we could support np as a magically param but not really bother to rework existing tests with it—instead we use it as-and-when we feel it useful to "sanity check" a test works the same for NumPy. So IMO it might be worth merging just the conftest.py changes.

@ev-br
Copy link
Collaborator

ev-br commented Mar 3, 2023

Merging conftest.py change (+explanation, this looks rather black magic to me at least :-)) LGTM. The rest, let's wait for Mario to weight in?

@honno honno marked this pull request as ready for review March 3, 2023 12:43
@honno honno requested a review from ev-br March 3, 2023 12:44
@ev-br ev-br merged commit 84c1fe1 into Quansight-Labs:main Mar 4, 2023
@ev-br
Copy link
Collaborator

ev-br commented Mar 4, 2023

Thank you @honno!
Let's track desired patterns of actually using this feature in #78

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