BUG [units]: fix signature incompatibilities between helper and helped functions by neutrinoceros · Pull Request #15710 · astropy/astropy

neutrinoceros · 2023-12-11T10:48:34Z

Description

Will close #15703
Opening as a draft with only the new tests. I'll address failures next.

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 · 2023-12-11T10:49:04Z

github-actions · 2023-12-11T10:49:09Z

👋 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 · 2023-12-11T13:08:55Z

I've started fixing clear incompatibilities discovered with my test. The remaining failures are a mixed bag and will require more careful examination, as I suspect there are some false positives.

neutrinoceros · 2023-12-11T13:54:59Z

I found that np.histogram, np.histogram2d and np.histogramdd underwent a unique change in numpy 1.24 as they lost their normed (positional-or-keyword) argument and the relative positions of density and weights (which are also positional-or-keyword) were swapped. This means that the simpler way to wrap them correctly in all supported versions is to use *args + **kwargs, but this would make their implementation more complicated. More importantly, the fact that density and weights are already present in the helper function's signature as positional-or-keyword, but with the pre-numpy 1.24 order makes it impossible to preserve 100% backward compatibility for astropy users and correctly replicate the current signature at the same time. Assuming that that usage of these function with 4 or 5 positional arguments is rare enough, maybe correctness should win over backward compatibility here ? I'm asking for @mhvk's guidance here.

neutrinoceros · 2023-12-11T15:24:20Z

I'm hitting some failures that I don't immediately understand that apparently show that my parametrised test behaves funky when ran in parallel sessions ?
Any help with that would be very much appreciated. Meanwhile, I'll go to pytest to report a seemingly different problem I've encountered while writing this patch.

hamogu · 2023-12-11T15:39:17Z

It is interesting that the error happens in the test collection, not when the test is actually run in parallel. Maybe that's a symptom of the same limitation that already led you to duplicate the parameterize fixture? You could probably bypass that (at the cost of some more duplicated lines of code) by breaking the class up into separate test functions?

neutrinoceros · 2023-12-11T15:40:58Z

It is interesting that the error happens in the test collection, not when the test is actually run in parallel. Maybe that's a symptom of the same limitation that already led you to duplicate the parameterize fixture?

yes I think it may be due to the fact that I used a consumable iterator (itertools.chain) for params. I think I fixed it now, let's see.

mhvk

Really nice! I need to look through the test function logic with perhaps a bit more detail, but I like how it already found quite a few little buglets (and numpy illogicalities!).

neutrinoceros · 2023-12-11T21:07:15Z

Thanks @mhvk for your review, I think I've addressed all your points !
Now the one thing we need to discuss is what to do about histogram functions (see my comment #15710 (comment)), since it blocks tests with oldest supported dependencies. Let me know what you think !

neutrinoceros · 2023-12-12T11:39:18Z

Actually I think I found a simple-ish solution for np.histogram and friends, and now CI is back to all-green !

mhvk

Just a few things; still have to go properly through the class you added (though it looks good!).

mhvk

OK, it is looking very good. Some comments on the actual test class, mostly to make it clearer (for me at least...)

Also, ideally the same tests are run for Masked, though I think it is fine to do that as follow-up...

mhvk · 2023-12-12T21:18:29Z

+                    assert (
+                        have_args_helper and have_kwargs_helper
+                    ), f"argument {nt!r} is not re-exposed as positional-or-keyword"
+            if pt.kind is VAR_POSITIONAL:


replace pt.kind with kt here too (or use pt.kind everywhere -- may be a bit clearer)

Also, this can be an elif

elif statements seem redundant with continue (which I just added following another of your suggestions !), and having them all be if makes it slightly more readable (to me ?) because it forces alignement and makes it easier to spot differences.

mhvk · 2023-12-12T21:20:18Z

+                    ), f"argument {nt!r} is not re-exposed as positional-or-keyword"
+            if pt.kind is VAR_POSITIONAL:
+                assert have_args_helper, "helper is missing a catch-all *args argument"
+            if pt.kind is VAR_KEYWORD:


Another elif

…ignatures TST: add meta tests for units.quantity_helper.function_helper to validate signature compatibility between helped and helper functions

neutrinoceros · 2023-12-13T14:07:09Z

@mhvk I just integrated all your recent suggestions in a rebase. To help you reviewing again, I kept all changes more recent than your review in a separate commit, but everything else was squashed !

Also, ideally the same tests are run for Masked, though I think it is fine to do that as follow-up...

I'll add it to my todo list, and will work on it after this one is dealt with !

mhvk

One little buglet, otherwise all nitpicks - should be ready to go in with these addressed.

mhvk · 2023-12-14T15:18:21Z


-@function_helper(helps=(np.isin, np.in1d))  # np.in1d deprecated in not NUMPY_LT_2_0.
-def setcheckop(ar1, ar2, *args, **kwargs):
+@function_helper  # np.in1d deprecated in not NUMPY_LT_2_0.


Here, the comment is misleading, since this is for isin

mhvk · 2023-12-14T15:19:05Z

-def setcheckop(ar1, ar2, *args, **kwargs):
+@function_helper  # np.in1d deprecated in not NUMPY_LT_2_0.
+def isin(element, test_elements, *args, **kwargs):
+    # This tests whether ar1 is in ar2, so we should change the unit of


I guess here and below we might as well adjust the names to the new ones.

mhvk · 2023-12-14T15:23:38Z

+        args_helper = list(params_helper.items())
+
+        pos_helper = 0
+        for nt, pt in args_target:


With the for loop, you can just replace with params_target.items() and remove the above definition.

mhvk · 2023-12-14T15:33:44Z

+        params_helper = sig_helper.parameters
+
+        for kind in (POSITIONAL_ONLY, POSITIONAL_OR_KEYWORD):
+            args_target = self.get_param_group(params_helper, [POSITIONAL_ONLY])


Here, you forgot to replace [POSITIONAL_ONLY] with [kind]!

good catch !

mhvk · 2023-12-14T15:36:12Z

+            if (nhelper := len(args_helper)) > (ntarget := len(args_target)):
+                unknown: list[str] = args_helper[ntarget:]
+                raise AssertionError(
+                    f"Found unknown {str(kind).lower().replace('_', '-')} parameter(s) "


For an error message, I'd not string-massage something that may change, i.e., just do unknown {kind} parameter(s)

I guess here the assert statement became just too unwieldy, right?

I guess here the assert statement became just too unwieldy, right?

Correct ! that's exactly why I used raise AssertionError 😉

neutrinoceros · 2023-12-14T16:29:53Z

@mhvk Thank you so much for your careful review. I think I've addressed or answered all your comments now.

mhvk

Super, looks all good. Thanks very much for creating this -- it should be a big help to keep things in sync.

pllim · 2023-12-14T20:50:20Z

Hmm do we have to backport this to make numpy-dev compat backports easier for 6.0.x?

mhvk · 2023-12-14T21:02:23Z

Hmm do we have to backport this to make numpy-dev compat backports easier for 6.0.x?

Good point. I think this is reasonably safe to backport, and it does fix one bug (new period argument for unwrap). From that respect, backporting seems sensible. But people have been a bit wary of anything labeled "API change"... (which in this case is extremely minor, but still). I guess the alternative would be to just backport the actual change. @neutrinoceros - what do you think?

neutrinoceros · 2023-12-14T22:07:08Z

To me it would make sense to backport, but it's also fine not to. If you guys think there's a chance that backporting would be controversial, I don't think it's worth it.

github-actions Bot added the units label Dec 11, 2023

neutrinoceros changed the title ~~TST: add meta tests for units.quantity_helper.function_helper to validate signature compatibility between helped and helper functions~~ BUG [units]: fix signature incompatibilities between helper and helped functions Dec 11, 2023

neutrinoceros force-pushed the tst/detect_incompatible_helper_signature branch from b8fc2b2 to 6047c74 Compare December 11, 2023 13:17

neutrinoceros force-pushed the tst/detect_incompatible_helper_signature branch from 6047c74 to 4a12859 Compare December 11, 2023 13:55

neutrinoceros commented Dec 11, 2023

View reviewed changes

Comment thread astropy/units/quantity_helper/function_helpers.py Resolved

neutrinoceros force-pushed the tst/detect_incompatible_helper_signature branch from 4a12859 to ad7be42 Compare December 11, 2023 14:23

pllim added this to the v6.1.0 milestone Dec 11, 2023

pllim added API change PRs and issues that change an existing API, possibly requiring a deprecation period Bug labels Dec 11, 2023

neutrinoceros force-pushed the tst/detect_incompatible_helper_signature branch from 28fdac2 to 95f4625 Compare December 11, 2023 18:11

mhvk reviewed Dec 11, 2023

View reviewed changes

neutrinoceros force-pushed the tst/detect_incompatible_helper_signature branch 3 times, most recently from 60e160f to 89fbf28 Compare December 12, 2023 11:38

neutrinoceros marked this pull request as ready for review December 12, 2023 11:39

mhvk reviewed Dec 12, 2023

View reviewed changes

Comment thread astropy/units/quantity_helper/function_helpers.py Resolved

Comment thread astropy/units/quantity_helper/function_helpers.py Resolved

Comment thread astropy/units/quantity_helper/function_helpers.py Resolved

mhvk reviewed Dec 12, 2023

View reviewed changes

neutrinoceros added 2 commits December 13, 2023 15:05

BUG: fix clear incompatibilities between helper and target function s…

917430e

…ignatures TST: add meta tests for units.quantity_helper.function_helper to validate signature compatibility between helped and helper functions

simplify test

d8e7942

neutrinoceros force-pushed the tst/detect_incompatible_helper_signature branch from 473300d to d8e7942 Compare December 13, 2023 14:05

neutrinoceros added 2 commits December 14, 2023 08:49

clarify comment

4c4d3d3

supplement comment

dcbf057

mhvk reviewed Dec 14, 2023

View reviewed changes

apply suggestions from review

32bed2c

mhvk approved these changes Dec 14, 2023

View reviewed changes

mhvk added the Refactoring label Dec 14, 2023

mhvk merged commit f87e75d into astropy:main Dec 14, 2023

neutrinoceros deleted the tst/detect_incompatible_helper_signature branch December 14, 2023 22:07

mhvk mentioned this pull request Jan 11, 2024

Proposal for helping keep __array_function__ overrides in sync #8937

Closed

neutrinoceros mentioned this pull request Feb 12, 2024

BUG: fix signature incompatibilities between helper and helped functions (astropy.utils.masked) #16025

Merged

1 task

neutrinoceros mentioned this pull request Apr 2, 2024

BUG: fix signature incompatibilities in NEP 18 wrapped functions yt-project/unyt#500

Merged

neutrinoceros mentioned this pull request Apr 11, 2024

Add type annotations to utils/shape.py #16282

Merged

1 task done

neutrinoceros mentioned this pull request Nov 6, 2025

TST: consider failing signature tests for numpy functions whose signatures are not accessible at runtime #18842

Closed

Conversation

neutrinoceros commented Dec 11, 2023

Description

Uh oh!

github-actions Bot commented Dec 11, 2023

Uh oh!

github-actions Bot commented Dec 11, 2023

Uh oh!

neutrinoceros commented Dec 11, 2023

Uh oh!

neutrinoceros commented Dec 11, 2023

Uh oh!

Uh oh!

neutrinoceros commented Dec 11, 2023

Uh oh!

hamogu commented Dec 11, 2023

Uh oh!

neutrinoceros commented Dec 11, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Dec 11, 2023

Uh oh!

neutrinoceros commented Dec 12, 2023

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

neutrinoceros commented Dec 13, 2023

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!

Uh oh!

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!