extra.array_api versioning + complex support#3456
extra.array_api versioning + complex support#3456Zac-HD merged 62 commits intoHypothesisWorks:masterfrom
extra.array_api versioning + complex support#3456Conversation
Zac-HD
left a comment
There was a problem hiding this comment.
At a high level, all your comments sound sensible to me - so let's charge ahead with the PR!
Some quick comments below, though as noted I'm not going to go into much depth until you ask or mark it ready-for-review. Finally, thanks again for maintaining this component!
4a703af to
93b16c5
Compare
|
Thanks for the suggestions! Other than an updated TODO list, think I'm now happy with the API and general architecture things, but something to sleep on. I updated the |
|
Having gone through it again, I don't think there are any major changes; just a little polish and I'll be happy to merge.
I'm fine with leaving a note (see comment above), and otherwise deciding that we'll deal with this later. More, smaller PRs is good! |
|
So in regards to For the Windows tests seem to be failing because the custom markers registered in |
Zac-HD
left a comment
There was a problem hiding this comment.
I think after this round we're probably just waiting for a decision on complex-dtype introspection? I'm looking forward to merging!
Windows won't pick on it otherwise
|
Thanks for the great review comments, should be all addressed now.
Technically this PR is compliant to the current draft spec, so it seems waiting is a question of do we want an inevitable future PR. I'm 99% sure complex support for Also I noticed |
Absolutely, also happy to support in |
Zac-HD
left a comment
There was a problem hiding this comment.
Technically this PR is compliant to the current draft spec, so it seems waiting is a question of do we want an inevitable future PR.
I'm a big fan of smaller PRs even if that means more PRs, so on your word that that this is compliant I'm happy to ship it now 🎉
879e9b7 to
bc4bcb6
Compare
|
🚢🚢:shipit: |
|
Awesome, thanks again Zac! |
PR to resolve #3422. That means 1) complex support 2) versioning namespaces. Not particularly looking for a thorough review just yet, but would of-course appreciate feedback and ideas.
Prior brain dump
Complex support
Per point 1, supporting complex arrays has consisted of my initial proposal
Versioned strategy namespaces
Per point 2,
make_strategies_namespace()now takes anapi_versionkw-only argument per @Zac-HD's suggestion. The argument..., which indeed tries known versions (descending) and returns the first that works."2021.12"for now), which forcibly returns a strategies namespace for the given version."draft", which lets us access a namespace with no guarantees (documented as such). The expectation is that the namespace would follow the draft spec. This is useful to implement and internally test features before spec releases (e.g. complex numbers)... and helps me out witharray-api-tests😅None, raises a hopefully useful error. Might need to mull it over, and write additional context if we detectNone.Currently in a
2021.12namespace,numeric_dtypes()doesn't generate complex numbers, andcomplex_dtypes()doesn't exist.draftconversely has the complex support as outlined a bit above.Internal test suite
The big TODO is updating the test suite. As of writing, currently I've introduced xp-agnostic tests for
make_strategies_namespace(), and tests for new complex behaviour.Parametrizing versioned namespaces?
Per
tests/array_api/conftest.py(see #3085 and #3275), we need to consider how versioning interacts with parametrization of array modules. See its README.I think ideally
make_strategies_namespace(...).mock_xpon"draft". Currently we test with NumPy-proper if available, but this is limiting in testing newer features (e.g. complex-numbers). Or maybe test bothmock_xpon"draft"andnumpyon inferred latest version.Version-specific testing
Mark tests which are only for YYYY.MM+ versions?
For the complex-related tests which get parametrize via
contest.py, my idea was/is to introduce a markerxp_min_version(api_version), e.g.I thought I could get the
xpsargument from a pytest item in thecollection_modifyitemshook, which would hold a newly introducedapi_versionattribute. For items withxp_min_version()markers, thatxps.api_versionvalue could of been checked against the min version (e.g. from<marker>.args[0]), where when "less-than" askipmarker would be added to the item... my first go didn't work out heh, so will sleep on it.For some tests, change test behaviour on runtime?
Also additionally
xps.api_versioncould maybe change behaviour of tests likeMight want to create separate tests, will mull over.
TODO:
RELEASE.rstand other things CI picks on__array_api_version__instead probably