-
Notifications
You must be signed in to change notification settings - Fork 591
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
extra.array_api
versioning + complex support
#3456
Conversation
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.
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 |
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.
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 |
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.
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_version
kw-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.12
namespace,numeric_dtypes()
doesn't generate complex numbers, andcomplex_dtypes()
doesn't exist.draft
conversely 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_xp
on"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_xp
on"draft"
andnumpy
on 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
xps
argument from a pytest item in thecollection_modifyitems
hook, which would hold a newly introducedapi_version
attribute. For items withxp_min_version()
markers, thatxps.api_version
value could of been checked against the min version (e.g. from<marker>.args[0]
), where when "less-than" askip
marker 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_version
could maybe change behaviour of tests likeMight want to create separate tests, will mull over.
TODO:
RELEASE.rst
and other things CI picks on__array_api_version__
instead probably