Clarify exception message for non-quantity input to ufuncs by mhvk · Pull Request #16878 · astropy/astropy

mhvk · 2024-08-25T18:43:52Z

This pull request is to improve the puzzling error message one can get if passing in both numbers and quantities to ufuncs, by adding an explanation (on purpose, the exception type and start of the message are not changed, to ensure this is not an API change -- there were tests in the unit package relying on it). cc @maxnoe

Before:

from erfa.ufunc import s2p
import astropy.units as u
s2p(0.5, 0.5, 5 * u.km)
AttributeError: 'NoneType' object has no attribute 'get_converter'

Now

AttributeError: 'NoneType' object has no attribute 'get_converter' (input without a 'unit' attribute? Such input is treated as dimensionless and cannot be converted to rad).

Fixes #16873

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-08-25T18:44:17Z

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.

nstarman

Nice! The parenthetical improves the error message a lot.
I would suggest adding a TODO item to convert this to an Exception Note (https://docs.python.org/3/tutorial/errors.html#enriching-exceptions-with-notes) for py3.11+. This is a nearly tailor-made example.

nstarman · 2024-08-25T19:17:38Z

astropy/units/tests/test_quantity_erfa_ufuncs.py


+    def test_s2p_not_all_quantity(self):
+        # Test for a useful error message - see gh-16873.
+        # Non-quantity input should be treated as dimensioness and thus cannot


Suggested change

# Non-quantity input should be treated as dimensioness and thus cannot

# Non-quantity input should be treated as dimensionless and thus cannot

nstarman · 2024-08-25T19:17:56Z

astropy/units/tests/test_quantity_erfa_ufuncs.py

+    def test_s2p_not_all_quantity(self):
+        # Test for a useful error message - see gh-16873.
+        # Non-quantity input should be treated as dimensioness and thus cannot
+        # be converted to radian.


Suggested change

# be converted to radian.

# be converted to radians.

nstarman · 2024-08-25T19:18:56Z

astropy/units/tests/test_quantity_erfa_ufuncs.py

        """Test unit errors when dimensionless parameters are used"""

-        msg = "'NoneType' object has no attribute 'get_converter'"
+        msg = "'NoneType'.*no attribute 'get_converter'.*treated as dimensionless"


Nifty! I forgot about the regex-capable input.

nstarman · 2024-08-25T19:19:07Z

astropy/units/tests/test_quantity_ufuncs.py


+    def test_sin_with_quantity_out(self):
+        # Test for a useful error message - see gh-16873.
+        # Non-quantity input should be treated as dimensioness and thus cannot


Suggested change

# Non-quantity input should be treated as dimensioness and thus cannot

# Non-quantity input should be treated as dimensionless and thus cannot

nstarman · 2024-08-25T19:19:12Z

astropy/units/tests/test_quantity_ufuncs.py

+    def test_sin_with_quantity_out(self):
+        # Test for a useful error message - see gh-16873.
+        # Non-quantity input should be treated as dimensioness and thus cannot
+        # be converted to radian.


Suggested change

# be converted to radian.

# be converted to radians.

mhvk · 2024-08-25T19:58:15Z

I would suggest adding a TODO item to convert this to an Exception Note (https://docs.python.org/3/tutorial/errors.html#enriching-exceptions-with-notes) for py3.11+. This is a nearly tailor-made example.

Very nice! I went ahead and just used it, with a version check that makes the new exception message match it also for python < 3.11. Also did the other small stuff (how did I not see those??)

maxnoe · 2024-08-25T20:06:29Z

astropy/units/quantity_helper/helpers.py

+    try:
+        converter = from_unit.get_converter(to_unit)
+    except AttributeError as e:
+        # Check for lack of unit only now, to avoid delay for cases where a unit


Double unit here (last word in this line, first in the next)

Oops, another typo, thanks for catching it... Fix in #16881

…itless-input-fixup Fix comment added in gh-16878 [skip ci]

pllim · 2024-08-26T18:08:42Z

astropy/units/quantity_helper/helpers.py

+                f"as dimensionless and cannot be converted to {to_unit}."
+            )
+            if sys.version_info >= (3, 11):
+                e.add_note(note)


Belated comment: I don't know how to feel about having error behavior be different based on Python version. But probably okay for this one case. 🤷‍♀️

I did check that the actual error message is exactly identical -- I thought it was easier to do it like this rather than wait for our minimum version of python to be 3.11.

But, actually, aren't we dropping support of python 3.10 by October (following SPEC 0)? If so, isn't this OK regardless?

But why is "exotic archs" on Python 3.12 failing? See #16878 (review)

pllim · 2024-08-26T19:17:53Z

astropy/units/tests/test_quantity_erfa_ufuncs.py

        """Test unit errors when dimensionless parameters are used"""

-        msg = "'NoneType' object has no attribute 'get_converter'"
+        msg = "'NoneType'.*no attribute 'get_converter'.*\n.*treated as dimensionless"


I think this broke "exotic archs" regex. See https://github.com/astropy/astropy/actions/runs/10554512360/job/29236483624

def test_unit_errors(self): """Test unit errors when dimensionless parameters are used""" msg = "'NoneType'.*no attribute 'get_converter'.*\n.*treated as dimensionless" > with pytest.raises(AttributeError, match=msg): E AssertionError: Regex pattern did not match. E Regex: "'NoneType'.*no attribute 'get_converter'.*\n.*treated as dimensionless" E Input: "'NoneType' object has no attribute 'get_converter'" astropy/units/tests/test_quantity_erfa_ufuncs.py:546: AssertionError

Anyways, I opened #16884 as follow-up issue that would block v7.0 release. If no one has idea how to fix, my proposal would be to revert the exception behavior changes (but keep your clearer message). Thanks!

mhvk added Docs units no-changelog-entry-needed labels Aug 25, 2024

mhvk added this to the v7.0.0 milestone Aug 25, 2024

mhvk requested a review from nstarman August 25, 2024 18:43

mhvk mentioned this pull request Aug 25, 2024

erfa.p2s does not accept units except for distance #16873

Closed

nstarman approved these changes Aug 25, 2024

View reviewed changes

Clarify exception message for non-quantity input to ufuncs

9918f83

mhvk force-pushed the units-better-error-message-for-unitless-input branch from f5efc6d to 9918f83 Compare August 25, 2024 19:56

mhvk enabled auto-merge August 25, 2024 19:59

maxnoe reviewed Aug 25, 2024

View reviewed changes

mhvk merged commit eaeaa5b into astropy:main Aug 25, 2024

nstarman mentioned this pull request Aug 25, 2024

Use Exceptions with Notes #16879

Open

mhvk added a commit to mhvk/astropy that referenced this pull request Aug 26, 2024

Fix comment added in astropygh-16878 [skip ci]

4f47b38

mhvk mentioned this pull request Aug 26, 2024

Fix comment added in gh-16878 [skip ci] #16881

Merged

pllim added a commit that referenced this pull request Aug 26, 2024

Merge pull request #16881 from mhvk/units-better-error-message-for-un…

584a2ee

…itless-input-fixup Fix comment added in gh-16878 [skip ci]

pllim reviewed Aug 26, 2024

View reviewed changes

pllim mentioned this pull request Aug 26, 2024

TST: "Exotic archs" has 3 failures with AssertionError: Regex pattern did not match. #16884

Closed

mhvk deleted the units-better-error-message-for-unitless-input branch February 17, 2025 17:41

	# Non-quantity input should be treated as dimensioness and thus cannot
	# Non-quantity input should be treated as dimensionless and thus cannot

Uh oh!

Conversation

mhvk commented Aug 25, 2024

Uh oh!

github-actions bot commented Aug 25, 2024

Uh oh!

nstarman 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!

mhvk commented Aug 25, 2024

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants