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

extra.array_api versioning + complex support #3456

Merged
merged 62 commits into from Sep 29, 2022

Conversation

honno
Copy link
Member

@honno honno commented Sep 13, 2022

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 an api_version kw-only argument per @Zac-HD's suggestion. The argument

  • defaults to ..., which indeed tries known versions (descending) and returns the first that works.
  • can be a released versions string (i.e. just "2021.12" for now), which forcibly returns a strategies namespace for the given version.
  • can be "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 with array-api-tests 😅
  • Anything else, including None, raises a hopefully useful error. Might need to mull it over, and write additional context if we detect None.

Currently in a 2021.12 namespace, numeric_dtypes() doesn't generate complex numbers, and complex_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

  • when specifying an array module, default to just testing with the strategies namespace we get from make_strategies_namespace(...).
  • add support for specifying a specific version for said array module(s).
  • by default, test our 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 both mock_xp on "draft" and numpy 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 marker xp_min_version(api_version), e.g.

# test_scalar_dtypes.py
@pytest.mark.xp_min_version("draft")
def test_can_generate_complex_dtypes(xp, xps):
    ...

# test_arrays.py
dtype_name_params = ["bool",] + list(REAL_NAMES) + [
    pytest.param(n, marks=pytest.mark.xp_min_draft("draft")]
]

@pytest.mark.parametrize("dtype_name", dtype_name_params)
def test_draw_arrays_from_dtype(...):
    ...

@pytest.mark.parametrize("dtype_name", dtype_name_params)
def test_draw_arrays_from_scalar_names(...):
    ...

I thought I could get the xps argument from a pytest item in the collection_modifyitems hook, which would hold a newly introduced api_version attribute. For items with xp_min_version() markers, that xps.api_version value could of been checked against the min version (e.g. from <marker>.args[0]), where when "less-than" a skip 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 like

# test_scalar_dtypes.py
def test_can_generate_numeric_dtypes(xp, xps):
    if api_version_gt(xps.api_version, "2021.12"):
        dtype_names = <includes complex dtypes>
    else:
        dtype_names = <doesn't include complex dtypes>
    numeric_dtypes = [getattr(xp, n) for n in dtype_names]
    assert_all_examples(xps.numeric_dtypes(), lambda dtype: dtype in numeric_dtypes)

Might want to create separate tests, will mull over.

TODO:

  • Review the diff and see if there's anything I could clean up easily.
  • RELEASE.rst and other things CI picks on
  • Update the test suite.
  • Update/add some API docs.
  • Add test docstrings (namely to force me to review them heh).
  • Utilise __array_api_version__ instead probably

Copy link
Member

@Zac-HD Zac-HD left a 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!

hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
@honno honno marked this pull request as ready for review September 19, 2022 12:32
@honno
Copy link
Member Author

honno commented Sep 19, 2022

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 tests/array_api/ README which should explain my current solution to namespace version paramtrization. Basically by default the latest supported version is used, and a new env lets you specify a specific version. I didn't introduce an "all" option as ugh I need to think about it 🙃 I guess a future problem is you can end up not testing with all released versions by default, just for now on CI you should have draft covered (by the mock) and 2021.12 covered by numpy.array_api.

@honno honno requested a review from Zac-HD September 19, 2022 12:38
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/tests/array_api/README.md Show resolved Hide resolved
hypothesis-python/tests/array_api/common.py Outdated Show resolved Hide resolved
hypothesis-python/tests/array_api/conftest.py Outdated Show resolved Hide resolved
hypothesis-python/tests/array_api/test_scalar_dtypes.py Outdated Show resolved Hide resolved
@Zac-HD
Copy link
Member

Zac-HD commented Sep 20, 2022

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 updated the tests/array_api/ README which should explain my current solution to namespace version paramtrization. Basically by default the latest supported version is used, and a new env lets you specify a specific version. I didn't introduce an "all" option as ugh I need to think about it 🙃 I guess a future problem is you can end up not testing with all released versions by default, just for now on CI you should have draft covered (by the mock) and 2021.12 covered by numpy.array_api.

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!

@honno
Copy link
Member Author

honno commented Sep 23, 2022

So in regards to xp.__array_api_version__ #3456 (comment), the PR now uses that for inferrence, and ditches the funky x.__array_namespace__() method (save here in case). Tests have been updated accordingly.

For the test/array_api/ conf, I now just default to testing with the mock, as it seems rather bothersome to raise a warning or whatever for default runs of the test suite (for CI but also developer experience). README.md updated. Also numpy.array_api might not exist/be updated soon enough? numpy/numpy#22252

Windows tests seem to be failing because the custom markers registered in tests/array_api/conftest.py are well not being registered heh. Will explore next week.

Copy link
Member

@Zac-HD Zac-HD left a 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!

hypothesis-python/RELEASE.rst Outdated Show resolved Hide resolved
hypothesis-python/tests/array_api/conftest.py Outdated Show resolved Hide resolved
hypothesis-python/tests/array_api/conftest.py Outdated Show resolved Hide resolved
hypothesis-python/tests/array_api/conftest.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/extra/array_api.py Outdated Show resolved Hide resolved
hypothesis-python/tests/array_api/common.py Outdated Show resolved Hide resolved
@honno
Copy link
Member Author

honno commented Sep 27, 2022

Thanks for the great review comments, should be all addressed now.

I think after this round we're probably just waiting for a decision on complex-dtype introspection?

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 xp.finfo() (data-apis/array-api#484) will get merged, given we agreed upon it in a recent consortium meeting. Spec triage is always variable and our resident reviewer is currently out, so it might not get merged say this week... we could shoot the gun here even?

Also I noticed nps.from_dtype() currently doesn't support min/max magnitude arguments—once we resolve complex-dtype introspection, it's okay to support those args for xps.from_dtype() at least I assume?

@Zac-HD
Copy link
Member

Zac-HD commented Sep 28, 2022

I noticed nps.from_dtype() currently doesn't support min/max magnitude arguments—once we resolve complex-dtype introspection, it's okay to support those args for xps.from_dtype() at least I assume?

Absolutely, also happy to support in xps too when that makes sense - and since it's a logically separate feature, please post it as a separate PR!

Copy link
Member

@Zac-HD Zac-HD left a 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 🎉

@Zac-HD
Copy link
Member

Zac-HD commented Sep 29, 2022

🚢🚢:shipit:

@Zac-HD Zac-HD merged commit e242573 into HypothesisWorks:master Sep 29, 2022
@honno
Copy link
Member Author

honno commented Sep 29, 2022

Awesome, thanks again Zac!

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.

Support complex dtypes in extra.array_api
2 participants