ENH: implement `np.arange` dispatch for `Quantity` by neutrinoceros · Pull Request #17059 · astropy/astropy

neutrinoceros · 2024-09-23T14:30:42Z

Description

Implement a helper function for np.arange with like=Quantity(...).
Since the implementation turned out to be complex with many overrides to take into account, I'm opening this PR at an early stage for visibility.
Ref #17001

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

TODO:

handle completion tests (xref BUG: overridable functions from NEP 35 do not show up in ARRAY_FUNCTIONS numpy/numpy#27451)
flesh out all raise statements (write actual error messages)
improve test coverage
add a test case for like=Angle(...)
changelog

github-actions · 2024-09-23T14:31:14Z

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?
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.

github-actions · 2024-09-23T14:31:16Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

neutrinoceros · 2024-09-24T16:26:38Z

I think I may be done with discovering and fixing edge cases 🤞🏻
I will revisit tomorrow and undraft then if I still feel like I covered everything.

neutrinoceros · 2024-09-24T18:46:34Z

One more edge case that I'd like to phrase as a question to reviewers:
in #17001, we've settled for "the output should have the unit of the first argument", but it's not clear who's the "first" argument in case they're all passed by name. For instance, should the following calls return the same array ?

np.arange(start=1*u.s, stop=10*u.min)
np.arange(stop=10*u.min, start=1*u.s)

In my current implementation they do, because keywords arguments follow the order they appear in the signature, which is invariant.

I guess another question that emerges from this illustration is: should it be valid to specify start and stop with different units without specifying a step ?

neutrinoceros · 2024-09-24T19:07:20Z

+
+    # now we can bind positional arguments to their meaningful names
+    assert len(args) <= 4
+    match args:


I think this is the first time the requirements for an implementation are so complex that using match/case feels simpler than the equivalent forest of if/else , but I'm really glad we have it.

lpsinger

Remember to update https://docs.astropy.org/en/stable/known_issues.html#numpy-array-creation-functions-cannot-be-used-to-initialize-quantity.

mhvk

Wow! Some comments inline, but also a larger point: I wonder if we need to keep track of the unit of the argument of like (i.e., self in Quantity.__array_function__). For np.zeros and np.ones, it would certainly make most sense for the output to have the same unit as the argument of like. For np.arange, this is less clear since I think we want something like np.arange(0*u.m, 10*u.m, like=0*u.s) to succeed. But perhaps it should set the default unit?

I think we need some special logic in Quantity.__array_function__ that assigns the unit to self.unit if it is not already set, i.e., if the output unit is not a unit but some signal (cannot be None, which is already used for truly unit-less output, like from comparisons, but could perhaps be False or True or something else recognizable)

mhvk · 2024-09-25T13:37:46Z

+            if start is not None and stop is None:
+                start, stop = stop, start
+
+    has_out_unit = False


The loop looks a bit like _quantities2arrays, except it has to deal with None. One perhaps can reverse the operations here, as in

kwargs = {k: v for k, v in (("start", start), ("stop", stop), ("step", step)) if v is not None} new_values = _quantities2arrays(kwargs.values()) kwargs = {k: v for k, v in zip(kwargs.keys(), new_values)} kwargs["dtype"] = dtype

this should be addressed on my next push (but I'm riding a train so I might not succeed before I get off)

neutrinoceros · 2024-09-25T16:04:35Z

wonder if we need to keep track of the unit of the argument of like (i.e., self in Quantity.array_function). For np.zeros and np.ones, it would certainly make most sense for the output to have the same unit as the argument of like. For np.arange, this is less clear since I think we want something like np.arange(0u.m, 10u.m, like=0*u.s) to succeed. But perhaps it should set the default unit?

I would agree that using the unit from like by default could make sense, but I currently don't see how we can pass it down to our dispatch function.

Update: and now I read the read of the message I replied to 🤦🏻‍♂️
ok your suggestion makes sense. I can give it a try tomorrow !

neutrinoceros · 2024-09-25T16:47:41Z

Ok I think I've addressed all but your larger scope suggestion about capturing like's unit as use it as a default if needed, which I'll come back to tomorrow !

neutrinoceros · 2024-09-26T08:51:41Z

@lpsinger thanks for the reminder (#17059 (review)), I somehow didn't see your message yesterday but I'll address it now !

neutrinoceros · 2024-09-26T09:46:18Z

Re: #17059 (comment)

Conveniently I missed this suggestion yesterday, but I think it fits pretty well the more general problem I set out to deal with this morning (regarding somehow using the unit from the like argument as a default).
I find that the refactor suggested creates is unfortunately not a strict simplification:
_quantities2array internally uses _as_quantity, which will happily convert pure numbers as dimensionless quantities, so additional logic is still needed to distinguish the arguments (other than like) being trully dimensionless, e.g., 1*u.dimensionless_unscaled, or just plain numbers.

Since I think this also connects with my (currently unanswered) question in #17059 (comment), I think we need to discuss these (edge?) cases before we can move further. Let me summarize the questions to be resolved here:

should it be valid to specify start and stop with different units without specifying a step ?
if so, should the following calls return the same array (or should the order of the keyword argument dictate whice one dictates the output unit) ?

Q = 1 * u.km
np.arange(start=1*u.s, stop=10*u.min, like=Q)
np.arange(stop=10*u.min, start=1*u.s, like=Q)

should the following be valid (and what should the output unit be: dimensionless, or km) ?

Q = 1 * u.km
np.arange(10, like=Q)

mhvk · 2024-09-26T13:31:20Z

1,2: since stop is the only argument that has to be specified, I think its unit should take precedence, i.e., the output unit should always be the unit of stop.
3: A little trickier. I think by analogy of zeros_like, full_like, etc., the unit of Q should be the default. But then arguably the unit of Q should always be used, so maybe instead we should just refuse the temptation to guess (as you had it already).

So, I think one can still use _quantities2arrays, by ensuring that stop is passed in first and then set unit_from_first=True -- though annoyingly one does have to check stop is a Quantity, so perhaps it doesn't shorten what you have...

neutrinoceros · 2024-09-27T08:08:40Z

so maybe instead we should just refuse the temptation to guess

words to live by. I think it makes sense to require units be set on the stop argument for now. We can always add more flexibility in the future (but might regret doing so now). I'll give it another go now !

neutrinoceros · 2024-09-27T08:41:52Z

Alright, I've adjusted my test suite to our new requirements and applied your suggested refactor as best I could.
FYI I'll be back to my home-office next Wednesday, so I might not be able to push more changes before that.

mhvk

This looks great! Only nitpicks left.

mhvk · 2024-09-27T14:26:53Z


 """

+from __future__ import annotations


Not actually needed?

mhvk · 2024-09-27T14:30:25Z

+        if v is not None
+    }
+
+    new_values, out_unit = _quantities2arrays(*list(qty_kwargs.values()), unit_from_first=True)


No need for the list, i.e., just *qty_kwargs.values(), unit_from_first=True

mhvk · 2024-09-27T14:38:27Z

+        assert type(arr) is u.Quantity
+        assert arr.unit == u.radian
+        assert arr.dtype == expected.dtype
+        np.testing.assert_array_almost_equal(arr.to_value(u.arcsec), expected)


Might as well do an from numpy.testing import assert_array_almost_equal, assert_array_equal on top.

ah, actually it's already there. Let me simplify this !

mhvk · 2024-09-27T14:41:18Z

@@ -0,0 +1 @@
+Added a ``np.arange`` dispatch for ``Quantity``.


Maybe expand

Added a ``np.arange`` dispatch for ``Quantity`` (requires one to use ``like=<some_quantity>``).

Just to avoid people getting too excited...

mhvk · 2024-09-27T14:43:58Z

+    <Quantity [0. , 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9] cm>
+
+Also note that the unit of the output array is dictated by that of the ``stop``
+argument, and the data has a floating-point dtype. If an integer dtype is


I'd remove the actual example of getting integers out - I really do not want to encourage people to use it. Physical quantities are real! So just, and that, like for quantities generally, the data has floating-point dtype by default.

mhvk · 2024-09-27T14:44:45Z

+    <Quantity [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] mm>

-Workarounds include moving the units outside of the call to
+Alternatively, one may move the units outside of the call as


I think to was more appropriate here, though frankly fine to just remove, i.e., end with ... call::

neutrinoceros · 2024-09-28T07:17:52Z

Thanks Marten ! I think I've addressed everything !

mhvk

Great, all good now, thanks!

* ENH: implement np.arange dispatch for Quantity * also test output dtype * fix a broken case where dtype is passed positionally * simplify test suite * DOC: update docs * TST: adjust test suite to new requirements * RFC: refactor impl to match new requirements * TST: add a new test case * DOC: adjust docs * cleanup * address review comments

github-actions Bot added the units label Sep 23, 2024

neutrinoceros mentioned this pull request Sep 23, 2024

Investigate enabling array creation routines for quantities #17001

Closed

neutrinoceros force-pushed the units/17001/impl_np_arange branch 2 times, most recently from 4013170 to ecd9c72 Compare September 23, 2024 14:33

pllim added this to the v7.0.0 milestone Sep 23, 2024

neutrinoceros changed the title ~~ENH: implement np.arange dispatch for Quantity~~ ENH: implement np.arange dispatch for Quantity Sep 23, 2024

neutrinoceros mentioned this pull request Sep 24, 2024

DEP: Can we drop support for Python 3.9 ? yt-project/unyt#519

Closed

neutrinoceros force-pushed the units/17001/impl_np_arange branch 2 times, most recently from de78fcf to 288f351 Compare September 24, 2024 09:19

neutrinoceros commented Sep 24, 2024

View reviewed changes

Comment thread astropy/units/tests/test_quantity_non_ufuncs.py Resolved

neutrinoceros force-pushed the units/17001/impl_np_arange branch 2 times, most recently from 3d5b004 to f61b81e Compare September 24, 2024 16:23

neutrinoceros force-pushed the units/17001/impl_np_arange branch 3 times, most recently from c3aee78 to a96e31f Compare September 24, 2024 17:24

neutrinoceros force-pushed the units/17001/impl_np_arange branch from a96e31f to b7f0882 Compare September 24, 2024 19:02

neutrinoceros commented Sep 24, 2024

View reviewed changes

ENH: implement np.arange dispatch for Quantity

c59a4b9

neutrinoceros force-pushed the units/17001/impl_np_arange branch from b7f0882 to c59a4b9 Compare September 25, 2024 08:56

neutrinoceros marked this pull request as ready for review September 25, 2024 08:56

neutrinoceros mentioned this pull request Sep 25, 2024

Improve performance of np.arange(Time, Time, TimeDelta) #12700

Open

lpsinger suggested changes Sep 25, 2024

View reviewed changes

mhvk reviewed Sep 25, 2024

View reviewed changes

neutrinoceros added 2 commits September 25, 2024 18:39

also test output dtype

86ade5d

fix a broken case where dtype is passed positionally

96c03f2

simplify test suite

e5be081

DOC: update docs

68dd075

neutrinoceros added 3 commits September 27, 2024 10:33

TST: adjust test suite to new requirements

3bd4a35

RFC: refactor impl to match new requirements

09a0791

TST: add a new test case

a63042e

neutrinoceros added 2 commits September 27, 2024 11:05

DOC: adjust docs

5e5cff3

cleanup

405ec4d

mhvk reviewed Sep 27, 2024

View reviewed changes

address review comments

5df4ebe

mhvk approved these changes Sep 28, 2024

View reviewed changes

mhvk merged commit 10da0ee into astropy:main Sep 28, 2024

neutrinoceros deleted the units/17001/impl_np_arange branch September 29, 2024 08:11

neutrinoceros added a commit to neutrinoceros/astropy that referenced this pull request Oct 3, 2024

RFC: cleanup unused logic bits from astropy#17059

6a2fd9a

This was referenced Oct 3, 2024

RFC: cleanup unused logic bits from #17059 #17117

Closed

ENH: add support for NEP 35 array constructors (empty, ones, zeros and full) with like=Quantity(...)) #17120

Merged

NEP35 implementation tracker #17124

Open

neutrinoceros mentioned this pull request Nov 8, 2025

BUG: fix forward compatibility for np.arange(<Quantity>, ...) against numpy 2.4 (dev) #18849

Merged

1 task

		@@ -0,0 +1 @@
		Added a ``np.arange`` dispatch for ``Quantity``.

Conversation

neutrinoceros commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions Bot commented Sep 23, 2024

Uh oh!

github-actions Bot commented Sep 23, 2024

Uh oh!

Uh oh!

neutrinoceros commented Sep 24, 2024

Uh oh!

neutrinoceros commented Sep 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpsinger left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhvk Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neutrinoceros commented Sep 25, 2024

Uh oh!

neutrinoceros commented Sep 26, 2024

Uh oh!

neutrinoceros commented Sep 26, 2024

Uh oh!

mhvk commented Sep 26, 2024

Uh oh!

neutrinoceros commented Sep 27, 2024

Uh oh!

neutrinoceros commented Sep 27, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Sep 28, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

neutrinoceros commented Sep 23, 2024 •

edited

Loading

mhvk Sep 25, 2024 •

edited

Loading

neutrinoceros commented Sep 25, 2024 •

edited

Loading