DEPR: deprecate `astropy.utils.check_broadcast` by neutrinoceros · Pull Request #16346 · astropy/astropy

neutrinoceros · 2024-04-26T06:23:52Z

Description

Fixes #15211 and replaces #15499 (I marked @felipekhouri as a co-author here)

I note that this implies IncompatibleShapeError is now unused (outside check_broadcast. Since users might be importing it to construct try/except blocks around check_broadcast, it should probably be deprecated too. Let me know if this should be done in the same PR or as a follow up.

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-04-26T06:24:16Z

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-04-26T06:24:18Z

👋 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-04-26T06:32:32Z

+                r"arg (?P<argno_b>\d+) with shape (?P<shape_b>\((\d+(, ?)?)+\))\."
+            )
+            if (match := broadcast_shapes_error_re.fullmatch(str(exc))) is None:
+                warnings.warn(


Since parsing the error message emitted by numpy is intrinsically fragile (but probably our least-bad option), I spent extra effort on making this logic as robust as possible: failing to parse an error message should be recoverable.

neutrinoceros · 2024-04-26T11:18:03Z

+def _add_note_to_exception(exc: Exception, note: str) -> Exception:
+    import sys
+
+    if sys.version_info >= (3, 11):
+        exc.add_note(note)
+        return exc
+    else:
+        # mimic Python 3.11 behavior:
+        # preserve error message and traceback
+        return type(exc)(f"{exc!s}\n{note}").with_traceback(exc.__traceback__)


This is a side improvement but isn't strictly needed here, I just applied an existing snippet of mine here. Let me know if I should revert it or move it to a separate PR.

I sligtly like preserving the same instance. In other code, I've done the equivalent of exc.args += ("\n" + note,). Would that work here?

Hum, I didn't think about that. I guess replacing a tuple attribute is objectively less evil than returning an entirely new instance in a context where we accept mutations anyway. Thanks !

mhvk

Thanks, this looks mostly excellent. But I really would prefer no code changes/comments just for test behaviour - hence the query whether that could be dealt with in the tests themselves. The rest is mostly small stuff.

mhvk · 2024-04-27T13:46:55Z

            try:
-                broadcast_shape = check_broadcast(*broadcasted_shapes[0])
-            except (IndexError, TypeError):
+                # always promote deprecation warnings to exceptions


Is this really necessary? I feel the real code should not have to worry about this. Is it not possible to adjust a test?

p.s. Wish I could trust that except Exception was not covered by the tests...

Is this really necessary?

For reference, the one test that failed before I applied this change was modeling/tests/test_core.py::test_inputless_model; It failed only in the context of wheel testing where the relevant difference with regular CI is that warnings are not elevated to errors.
See example logs
I haven't found a way to adjust the test itself yet, but I agree that this patch leaves a bad taste. I'll circle back to this.

p.s. Wish I could trust that except Exception was not covered by the tests...

From my experience running this is CI, it is covered, or I wouldn't have needed the aforementioned patch.

note to self: I think Model._prepare_inputs_single_model underwent an implicit change of behavior and needs adjusting.

I just pushed what I think is a more maintainable solution, together with a companion comment explaining my findings.

mhvk · 2024-04-27T13:51:56Z

-            r" 'p1' of shape .*"
-        )
-        with pytest.raises(ValueError, match=MESSAGE):
+        with pytest.raises(ValueError):


I never liked full matching of error messages, but ValueError is very broad, so maybe keep a simple match="broadcast"? (here and elsewhere in file)

mhvk · 2024-04-27T13:52:56Z

-    with pytest.raises(
-        ValueError, match="All inputs must have identical shapes or must be scalars"
-    ):
+    with pytest.raises(ValueError):


Maybe again a match="broadcast"

mhvk · 2024-04-27T13:56:43Z

+def _add_note_to_exception(exc: Exception, note: str) -> Exception:
+    import sys
+
+    if sys.version_info >= (3, 11):
+        exc.add_note(note)
+        return exc
+    else:
+        # mimic Python 3.11 behavior:
+        # preserve error message and traceback
+        return type(exc)(f"{exc!s}\n{note}").with_traceback(exc.__traceback__)


I sligtly like preserving the same instance. In other code, I've done the equivalent of exc.args += ("\n" + note,). Would that work here?

neutrinoceros · 2024-04-27T16:32:18Z

@mhvk thanks for your review. I think I addressed all your minor comments. I'll circle back to the more important one on Monday !

mhvk

Looks good!

Final question: did you try ignoring all warnings in test_check_broadcast? If that also doesn't work, then let's get things in as they are. Thanks!

neutrinoceros · 2024-04-29T07:16:03Z

Thanks for reminding me. I updated the test with your suggestion then squashed all commits together.

Co-authored-by: Felipe Gameleira <[email protected]>

mhvk

Super, let's get it in!

github-actions Bot added coordinates modeling utils labels Apr 26, 2024

neutrinoceros force-pushed the depr/deprecate_check_broadcast branch from a99383f to 8692aa8 Compare April 26, 2024 06:24