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

✨ feat: add Quantity API #16

Merged
merged 7 commits into from
Dec 5, 2024
Merged

✨ feat: add Quantity API #16

merged 7 commits into from
Dec 5, 2024

Conversation

nstarman
Copy link
Member

@nstarman nstarman commented Dec 3, 2024

This PR adds 2 runtime-checkable protocols that define the minimum Q API and the more complete API that includes the Array API. These protocols are made public in a new module named api.

private protocols and API are defined in the private modules _src/array_api and _src/api. The former is a temporary module that we can deprecate when the https://github.com/data-apis/array-api-typing package is developed and released.

Please squash-merge.

@nstarman nstarman marked this pull request as ready for review December 3, 2024 17:14
@nstarman
Copy link
Member Author

nstarman commented Dec 3, 2024

@mhvk are you good to ignore RUF022? The __all__ are currently topically sorted.

@nstarman nstarman requested a review from mhvk December 3, 2024 17:20
@mhvk
Copy link
Contributor

mhvk commented Dec 3, 2024

@mhvk are you good to ignore RUF022? The __all__ are currently topically sorted.

Sure, the sorting only starts to make sense when there are lots of entries. I don't think we're going to get there!

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, though with small queries inside. I do think if we expose anything, we should add tests for it. But I'm happy not to expose this yet, and instead just use it in the typing PR.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have some tests. I made some in-line comments about testing u.Quantity and the various array types too, but then thought it was probably easier to just push an extra commit...

So, that leaves only a question about the __subclass_hook__ implementation. Will approve assuming that it is OK.

@nstarman
Copy link
Member Author

nstarman commented Dec 4, 2024

@mhvk I refactored the tests to use pytest fixtures. They should be the same, just avoiding playing with globals.

Signed-off-by: nstarman <[email protected]>
# ------------------------------


@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about a structure like this when I wrote test_operations, but I rather dislike that it requiers every method to have that explicit fixture: I think setup_class() is considerably nicer: one just does tests on a given setup, which includes a given array type. Unfortunately, I could not seem to find a clean way in which one can properly parametrize classes by their setup, though I'll admit that I've never really understood the whole fixture process -- I find them rather unreadable (it seems pytest makes up its own language within a language rather than just using standard python stuff).

Anyway, long & short, I'd prefer to stick to one way of parametrizing over the array namespaces, ideally one that doesn't require to do setup in each method...

Since that is somewhat orthogonal to the goals of this PR, OK to just push back what I had and merge? (I'm definitely not stuck to my class creation code, but would like something that does not add boilerplate to each test!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we can just make a class-scoped fixture (https://docs.pytest.org/en/7.1.x/how-to/fixtures.html#scope-sharing-fixtures-across-classes-modules-packages-or-session) by moving def array_and_quantity(request): into the class and adding the argument to the fixture. It neatly replaces class_setup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try running pytest in verbose mode, it now repeats the whole class as a block for each array library.

@nstarman
Copy link
Member Author

nstarman commented Dec 4, 2024

If you merge, plz squash-merge, editing the comment.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not completely sold on the fixture for the general case still, but here it is very obvious what it does, so let's just get it in. Thanks, @nstarman!

@mhvk mhvk merged commit 0e01a15 into astropy:main Dec 5, 2024
3 checks passed
@nstarman nstarman deleted the feat/api branch December 5, 2024 15:55
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