Skip to content

Let Quantity accept Array-API compatible objects - #15752

Merged
mhvk merged 1 commit into
astropy:mainfrom
MridulS:quantity-array-api
Dec 17, 2023
Merged

Let Quantity accept Array-API compatible objects#15752
mhvk merged 1 commit into
astropy:mainfrom
MridulS:quantity-array-api

Conversation

@MridulS

@MridulS MridulS commented Dec 15, 2023

Copy link
Copy Markdown
Contributor

This is a hack to make the code block in #15071 work and I just wanted to test this out and see how things need to work/refactor inside Quantity to work. The changes here are minimal and it just casts any array-api compatible object to numpy array (as suggested in #15071 (comment)), like a torch tensor or cupy array will be able to use astropy to create a Quantity object.

The following code would work with this change.

>>> import torch
>>> import astropy.units as u
>>> array = torch.asarray([1, 2, 3])
>>> u.Quantity(array)
<Quantity [1., 2., 3.]>

The quantity value in the above case loses the torch tensor as it's wrapped by a numpy ndarray.

>>> u.Quantity(array).value == array
False

Would there be interest in refactoring Quantity and making it work natively with array-api compatible types?

@github-actions

github-actions Bot commented Dec 15, 2023

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

pllim
pllim reviewed Dec 15, 2023
View reviewed changes

@pllim pllim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Left a general review.

I think this also needs a change log fragment but I cannot tell if it is feature or api, so I'll let units maintainers decide.

Comment thread astropy/units/quantity.py Resolved
Comment thread astropy/units/tests/test_quantity_array_methods.py Resolved
Comment thread astropy/units/tests/test_quantity_array_methods.py Resolved
@pllim

pllim commented Dec 15, 2023

Copy link
Copy Markdown
Member

Also the failure looks related: TypeError: object of type 'Array' has no len()

@pllim pllim added this to the v6.1.0 milestone Dec 15, 2023
pllim
pllim reviewed Dec 15, 2023
View reviewed changes
Comment thread astropy/units/tests/test_quantity_array_methods.py Resolved
mhvk
mhvk reviewed Dec 16, 2023
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR! Rather than checking for array API, I think it is better to avoid the failure that currently occurs, which can be done by replacing

if isiterable(value) and len(value) > 1

with

if isinstance(value, (list, tuple)) and len(value) > 1

I think this is actually a bit better than isiterable anyway, though it is an API change so will need a corresponding changelog entry...

Comment thread astropy/units/quantity.py Outdated
# To ensure array remains fast, we short-circuit it.
value_unit = None
if not isinstance(value, np.ndarray):
from astropy.utils.compat.optional_deps import HAS_ARRAY_API_COMPAT

@mhvk mhvk Dec 16, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we need this explicit test - this branch just tests for string and sequences of Quantity, and otherwise falls through to an np.array call which will already do the right thing. See main comment.

Comment thread astropy/units/tests/test_quantity_array_methods.py Resolved
Comment thread astropy/units/tests/test_quantity_array_methods.py Resolved
Comment thread astropy/units/tests/test_quantity_array_methods.py Resolved
Comment thread astropy/utils/compat/optional_deps.py Resolved
Comment thread pyproject.toml Resolved
@MridulS

MridulS commented Dec 17, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews! Now I just avoid the iterable check as suggested, this should fix #15071 but we would need array_api_compat if astropy wants to natively support other array types (currently it will cast them using ndarray). I'm not sure with the API changelog I have written so feel free to overwrite it :D

@MridulS @mhvk
Let Quantity accept Array-API compatible objects
def12c5 
If any iterable such as a list of tuple was input to ``Quantity``, a check was
done to see if they contained only quantities, and, if so, the quantities were
concatenated.  This makes sense for list and tuple, but is not necessarily
logical for all iterables and indeed was broken for those that do not have a
length (such as ``array_api`` array instances). Hence, the check will now be
done only for values where it makes sense, i.e., instances of list and tuple.
@mhvk
mhvk force-pushed the quantity-array-api branch from f676f24 to def12c5 Compare December 17, 2023 16:45
mhvk
mhvk approved these changes Dec 17, 2023
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, looks good! I rephrased the API changelog entry a bit, hopefully making it (even) clearer, and also squashed the commits. Approving now and setting up for auto-merge...

@mhvk mhvk added API change PRs and issues that change an existing API, possibly requiring a deprecation period Refactoring labels Dec 17, 2023
@mhvk
mhvk enabled auto-merge December 17, 2023 16:47
@mhvk
mhvk merged commit 436669c into astropy:main Dec 17, 2023
@MridulS MridulS mentioned this pull request Dec 21, 2023
Closed
@MridulS
MridulS deleted the quantity-array-api branch January 16, 2024 13:49
@MridulS MridulS mentioned this pull request Feb 18, 2024
Closed
@mhvk mhvk mentioned this pull request Mar 4, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

API change PRs and issues that change an existing API, possibly requiring a deprecation period installation Refactoring units

Projects

None yet

Milestone

v6.1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@MridulS @pllim @mhvk