ENH: add support for NEP 35 array constructors (`empty`, `ones`, `zeros` and `full`) with `like=Quantity(...)`) by neutrinoceros · Pull Request #17120 · astropy/astropy

neutrinoceros · 2024-10-03T13:38:01Z

Description

Follow up to #17059, fixes #17001
Note that this is not a complete implementation of NEP 35 functions, but it does cover everything that was listed on #17001. I plan to open another issue to track what's left to do.
While working on these functions, I found that the mechanism suggested by @mhvk in #17059 for defaulting the output unit to that of the like argument made most sense for functions that don't take other arguments for which a quantity is meaningful, so I implemented it here and retro-fitted it in my implementation for np.arange too, with the particularity that I'm passing it under a different, private name (_like) to better signal that this argument is internal and not passed down by numpy itself.

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.

github-actions · 2024-10-03T13:38:27Z

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-10-03T13:38:29Z

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

pllim · 2024-10-03T15:37:28Z

I'll let units maintainers to decide if this needs a What's New. Thanks!

mhvk

Looks good, but with a suggestion. Also, how about asarray?

mhvk · 2024-10-03T16:55:17Z


        elif function in DISPATCHED_FUNCTIONS:
            dispatched_function = DISPATCHED_FUNCTIONS[function]
+            if function in SUPPORTED_NEP35_FUNCTIONS:


If we instead move this to after the try/except, as

if unit is True and function in SUPPORTED_NEP35_FUNCTIONS: unit = self.unit

then we gain a tiny bit in performance, no longer need to adjust our test machinery, and there's no need for a private argument. I think at least at the moment that is fine, since all we ever get from self is the unit.

Done. I was confused at first by the use of True as a sentinel value, which I believe is sufficient proof that it's not very clear, so I added my own, but the logic is yours !

mhvk · 2024-10-03T16:57:27Z


-    if not isinstance(stop, Quantity):
-        raise TypeError(f"Expected stop to be a Quantity, got {stop=} ({type(stop)=}")
+    if isinstance(stop, Quantity):


With my logic, this would become

out_unit = getattr(stop, "unit", True)

(I think it is OK to just look for unit - say, if a Column was passed in. That's consistent with what _quantities2arrays does.)

mhvk · 2024-10-03T16:57:51Z

+        ret_arr = np.empty(shape, dtype, order)
+    else:
+        ret_arr = np.empty(shape, dtype, order, device=device)
+    return ret_arr, _like.unit, None


So, here it would be return ret_arr, True, None

mhvk · 2024-10-03T17:00:49Z

 all_wrapped_functions = get_wrapped_functions(
    np, np.fft, np.linalg, np.lib.recfunctions
 )
+all_wrapped_functions |= SUPPORTED_NEP35_FUNCTIONS


Can we add a comment referring to your numpy issue? Hopefully, we won't need to use SUPPORTED_NEP35_FUNCTIONS here eventually.

mhvk · 2024-10-03T17:01:03Z

                        f"helper is not re-exposing argument {nt!r} properly: "
                        f"expected {kt}, got {kh}"
                    )
+                elif nt == "like":


This could go with my suggestion.

not completely though: since re-exposing like doesn't really make sense, I'm still keeping this branch (simplified, though)

mhvk · 2024-10-03T17:02:34Z

I'll let units maintainers to decide if this needs a What's New. Thanks!

A what's new makes sense, if we support all of them!

neutrinoceros · 2024-10-03T17:47:38Z

A what's new makes sense, if we support all of them!

I can add this as a todo to the global issue I'll write tomorrow. Would that work ?

neutrinoceros · 2024-10-04T05:44:46Z

how about asarray?

I'm focusing on what was requested in #17001 to avoid bloating this PR for now, and asarray is part of a subset of functions that I think belong together (array, asarray, asanyarray, ascontiguousarray and asfortranarray) and plan to tackle next.

neutrinoceros · 2024-10-04T06:09:15Z

Alright, I implemented @mhvk's suggestion, so this should be ready to go. I'll also write the follow up issue now.
update: here it is #17124

neutrinoceros · 2024-10-04T06:44:38Z

I almost forgot to also update documentation, but it's done now

mhvk

Thanks! One larger worry about the use of the unit in arange, otherwise small bits & pieces.

mhvk · 2024-10-04T14:07:15Z

    }

-    new_values, out_unit = _quantities2arrays(*qty_kwargs.values(), unit_from_first=True)
+    new_values, _ = _quantities2arrays(*qty_kwargs.values())


Thinking about this, I think you still need unit_from_first, otherwise if stop has no unit and start has one, that unit from start will be taken but discarded here. I think it may be best to have an explicit test that if stop has no unit, all other values also have no unit. That probably can then remain a separate branch, with this piece only happening for the case that stop has a unit.

I'm not sure I got what you mean here. I made an attempt (which I'm about to push), but it still doesn't require unit_from_first.

mhvk · 2024-10-04T14:08:26Z

    else:
        return np_core.shape_base._atleast_nd(arrays, result_ndim)

+_UnitFromLikeArg = object()


Why the camel case? I thought that was meant more for classes. Just unit_from_like or UNIT_FROM_LIKE (no real reason for it to be private, I think; none of these are exported up).

Why the camel case?

I don't know 😅

mhvk · 2024-10-04T14:09:33Z

+    def empty(shape, dtype=None, order='C', *, device=None):
+        return empty_impl(shape, dtype, order, device=device)
+
+def empty_impl(shape, dtype=None, order='C', *, device=None):


This implementation function is super short and still has to distinguish between numpy versions - just put them directly in the functions above?

mhvk · 2024-10-04T14:10:20Z

+    def ones(shape, dtype=None, order='C', *, device=None):
+        return ones_impl(shape, dtype, order, device=device)
+
+def ones_impl(shape, dtype=None, order='C', *, device=None):


Same as above. Alternatively, one could have an implementation for all those creation functions (i.e., also pass a function=np.ones along).

mhvk · 2024-10-04T14:11:07Z

+    return ret_arr, _UnitFromLikeArg, None
+
+@dispatched_function
+def zeros(shape, dtype=None, order='C'):


Does zeros not have the device argument?

It doesn't. Otherwise we'd be seeing at least one test fail, but I admit I too was surprised that its signature differs from that of ones. It's probably either an oversight or reflects that ones is part of the Array API and zeros isn't.

mhvk · 2024-10-04T14:11:43Z

+    def full(shape, fill_value, dtype=None, order='C', *, device=None):
+        return full_impl(shape, fill_value, dtype, order, device=device)
+
+def full_impl(shape, fill_value, dtype=None, order='C', *, device=None):


Here one could still call the overall implementation, and then get the unit after.

I don't feel comfortable generalizing early as I worry we could get trapped when later expanding the list of implemented function, so I'd like to avoid designing a general implementation for now if that's okay.

mhvk · 2024-10-04T15:07:16Z

@neutrinoceros - I made a quick PR to yours with an implementation using function_helper - which is really the one that is supposed to deal with cases where functions are very similar. See neutrinoceros#25

…d full) with like=Quantity(...)

neutrinoceros · 2024-10-04T15:34:27Z

@mhvk here's my attempt to reply to your latest review, but there may still be a couple points to discuss.

mhvk · 2024-10-04T17:49:23Z

@neutrinoceros - I rebased my PR to your PR

Use function_helper

neutrinoceros · 2024-10-04T18:54:42Z

aaaand merged. Thanks !

mhvk

All looks good now, but one suggestion still in-line (though fine to merge without it).

mhvk · 2024-10-04T19:55:10Z

        assert b.unit == u.min
        assert_array_equal(a.value, b.value)

+    def test_empty(self):


You could factor out the test by making a new class

class TestArrayCreation(BasicTestSetup): def check(self, func, *args, **kwargs): o = func(*args, **kwargs, like=self.q) assert type(o) is type(self.q) assert o.unit == self.q.unit expected = func(*args, **kwargs) << self.q.unit assert_array_equal(o, expected)

Then only for full do you need to keep the test with fill_value with a unit.

for reference, I've integrated this suggestion into #17125

neutrinoceros · 2024-10-04T20:35:49Z

If that's okay I'd prefer to apply your suggestion in a follow up PR, so I can rebase #17125 on main sooner rather than later and avoid stacking any further. Thanks !

mhvk · 2024-10-04T21:03:40Z

OK, let's get this one in!

github-actions Bot added the units label Oct 3, 2024

neutrinoceros force-pushed the units/enh/impl_nep35_array_constructors branch from e438875 to 355af1a Compare October 3, 2024 13:39

neutrinoceros changed the title ~~ENH: add support for NEP 35 array constructors (empty, ones, zeros and full) with like=Quantity(...)~~ ENH: add support for NEP 35 array constructors (empty, ones, zeros and full) with like=Quantity(...) Oct 3, 2024

This was referenced Oct 3, 2024

Investigate enabling array creation routines for quantities #17001

Closed

RFC: cleanup unused logic bits from #17059 #17117

Closed

neutrinoceros force-pushed the units/enh/impl_nep35_array_constructors branch 2 times, most recently from 057e42c to 97499cd Compare October 3, 2024 14:01

neutrinoceros marked this pull request as ready for review October 3, 2024 14:15

neutrinoceros force-pushed the units/enh/impl_nep35_array_constructors branch from 97499cd to a540f7a Compare October 3, 2024 14:17

pllim added this to the v7.0.0 milestone Oct 3, 2024

pllim requested review from mhvk and nstarman October 3, 2024 15:33

neutrinoceros changed the title ~~ENH: add support for NEP 35 array constructors (empty, ones, zeros and full) with like=Quantity(...)~~ ENH: add support for NEP 35 array constructors (empty, ones, zeros and full) with like=Quantity(...)) Oct 3, 2024

mhvk reviewed Oct 3, 2024

View reviewed changes

pllim added the whatsnew-needed label Oct 3, 2024

neutrinoceros force-pushed the units/enh/impl_nep35_array_constructors branch 2 times, most recently from 0eaff36 to 16bf302 Compare October 4, 2024 06:08

neutrinoceros mentioned this pull request Oct 4, 2024

NEP35 implementation tracker #17124

Open

6 of 28 tasks

neutrinoceros force-pushed the units/enh/impl_nep35_array_constructors branch 2 times, most recently from 1ade8bd to a2bc0c5 Compare October 4, 2024 11:15

neutrinoceros mentioned this pull request Oct 4, 2024

ENH: add support for NEP 35 constructors (array, asarray, asanyarray, ascontiguousarray and asfortranarray) with like=Quantity(...)) #17125

Merged

1 task

mhvk reviewed Oct 4, 2024

View reviewed changes

mhvk added Enhancement and removed whatsnew-needed labels Oct 4, 2024

ENH: add support for NEP 35 array constructors (empty, ones, zeros an…

c286244

…d full) with like=Quantity(...)

neutrinoceros force-pushed the units/enh/impl_nep35_array_constructors branch from a2bc0c5 to c286244 Compare October 4, 2024 15:31

Use function_helper

c4ad99c

Merge pull request #25 from mhvk/pr-17120

5b4e6ab

Use function_helper

mhvk approved these changes Oct 4, 2024

View reviewed changes

mhvk merged commit a9bcdb3 into astropy:main Oct 4, 2024

neutrinoceros deleted the units/enh/impl_nep35_array_constructors branch October 5, 2024 05:16

Conversation

neutrinoceros commented Oct 3, 2024

Description

Uh oh!

github-actions Bot commented Oct 3, 2024

Uh oh!

github-actions Bot commented Oct 3, 2024

Uh oh!

pllim commented Oct 3, 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!

mhvk commented Oct 3, 2024

Uh oh!

neutrinoceros commented Oct 3, 2024

Uh oh!

neutrinoceros commented Oct 4, 2024

Uh oh!

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

Uh oh!

Uh oh!

neutrinoceros commented Oct 4, 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!

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!

mhvk commented Oct 4, 2024

Uh oh!

neutrinoceros commented Oct 4, 2024

Uh oh!

mhvk commented Oct 4, 2024

Uh oh!

neutrinoceros commented Oct 4, 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!

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

Uh oh!

Uh oh!

mhvk commented Oct 4, 2024

neutrinoceros commented Oct 4, 2024 •

edited

Loading

neutrinoceros commented Oct 4, 2024 •

edited

Loading