Use Exception.add_note() for enriched error context in nddata and coordinates by FazeelUsmani · Pull Request #18877 · astropy/astropy

FazeelUsmani · 2025-11-11T08:05:32Z

This PR adopts Python 3.11's Exception.add_note() method to enrich exception messages with contextual information, replacing the older exc.args += pattern.

Changes

astropy/nddata/utils.py: Updated extract_array() to use add_note() when fill_value type is incompatible
astropy/coordinates/earth.py: Updated gravitational_redshift() to use add_note() for mass/gravitational parameter unit errors
Added comprehensive tests to verify exception notes are properly attached

Testing

Added two new test functions:

test_extract_array_fill_value_exception_note(): Verifies notes in nddata.utils
test_gravitational_redshift_exception_note(): Verifies notes in coordinates.earth

References

Fixes #16879

Changelog Entry

astropy.nddata
^^^^^^^^^^^^^^

- Enhanced error messages in ``extract_array()`` using Python 3.11's 
  ``Exception.add_note()`` for better debugging context. [#16879]

astropy.coordinates
^^^^^^^^^^^^^^^^^^^

- Enhanced error messages in ``EarthLocation.gravitational_redshift()`` 
  using Python 3.11's ``Exception.add_note()`` for better debugging context. [#16879]

github-actions · 2025-11-11T08:05:43Z

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.

neutrinoceros

pytest 8.0 (which we require) supports matching exception notes with the match keyword argument in pytest.raises, please use that instead of capturing the exception manually.

See pytest-dev/pytest#11227

neutrinoceros · 2025-11-13T14:34:37Z

@FazeelUsmani could you avoid merging from main 4 times a day ? I get notified every single time and it's a waste of CI resources

FazeelUsmani · 2025-11-14T04:27:50Z

Okay, sure.

FazeelUsmani · 2025-11-27T13:22:32Z

Please review this PR

nstarman

Thanks @FazeelUsmani ! This looks like a nice addition.

astropy/coordinates/tests/test_earth.py

Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>

nstarman

Thanks @FazeelUsmani !
Ping @eerovaher @mhvk @mwcraig for final reviews.

mwcraig

Thanks for this @FazeelUsmani!

eerovaher · 2025-12-01T16:28:02Z

astropy/coordinates/tests/test_earth.py

+def test_gravitational_redshift_exception_note():
+    """Test that exception notes are added for UnitsError in gravitational_redshift."""
+    someloc = EarthLocation(lon=-87.7 * u.deg, lat=37 * u.deg)
+    sometime = Time("2017-8-21 18:26:40")
+
+    # Test with wrong units to trigger UnitsError
+    masses = {
+        "sun": constants.G * constants.M_sun,
+        "jupiter": 1 * u.km,  # wrong units - should be mass or G*mass
+    }
+
+    with pytest.raises(u.UnitsError, match="masses.*gravitational parameters"):
+        someloc.gravitational_redshift(sometime, masses=masses)
+
+


Suggested change

def test_gravitational_redshift_exception_note():

"""Test that exception notes are added for UnitsError in gravitational_redshift."""

someloc = EarthLocation(lon=-87.7 * u.deg, lat=37 * u.deg)

sometime = Time("2017-8-21 18:26:40")

# Test with wrong units to trigger UnitsError

masses = {

"sun": constants.G * constants.M_sun,

"jupiter": 1 * u.km, # wrong units - should be mass or G*mass

}

with pytest.raises(u.UnitsError, match="masses.*gravitational parameters"):

someloc.gravitational_redshift(sometime, masses=masses)

There is no need for a new test. The UnitsError is already being tested, so the existing test should be updated instead.

eerovaher · 2025-12-01T16:29:21Z

docs/changes/coordinates/18877.feature.rst

This does not need a change log entry.

Co-authored-by: Eero Vaher <eero.vaher@gmail.com>

eerovaher · 2025-12-02T14:15:52Z

astropy/nddata/tests/test_utils.py

+def test_extract_array_fill_value_exception_note():
+    """Test that exception notes are added when fill_value type is incompatible."""
+    data = np.arange(12, dtype=int).reshape(3, 4)
+
+    # Try to extract with partial mode and incompatible fill_value (NaN for integer array)
+    with pytest.raises(ValueError, match="fill_value.*data type"):
+        extract_array(data, (5, 5), (1, 1), mode="partial", fill_value=np.nan)


It shouldn't be my job to review nddata, but why is this new test needed if we already have

astropy/astropy/nddata/tests/test_utils.py

Lines 430 to 436 in 818c1ac

def test_extract_array_nan_fillvalue():

if Version(np.__version__) >= Version("1.20"):

msg = "fill_value cannot be set to np.nan if the input array has"

with pytest.raises(ValueError, match=msg):

extract_array(

np.ones((10, 10), dtype=int), (5, 5), (1, 1), fill_value=np.nan

)

If anything I would prefer that the new test replace the old test given that the new test explains what it is checking. I am also fine with both staying in there since they are not quite identical....hence the approval.

If @FazeelUsmani would like to replace the old test you cited with this new test I'm happy to approve that also.

I don't see what's the point of having two separate tests for checking a single static error message.

I think this PR is AI generated.

Yes, I used Claude code

Yes, I used Claude code

That in and of itself is not an issue -- I also use Claud Code. One needs to happen here is cleaning up the tests. As @eerovaher points out, the tests generated by Claude are already present elsewhere in the code (though I actually like Claude's version of the test in NDData more than the current test).

There are a couple paths forward here:

You can manually delete the two new tests from the pull request

You can ask Claude to delete them

I often find that I need to "coach" Claude to end up with a useful result on par with what a skilled programmer would produce.

Obligatory link to https://xkcd.com/1319/ (and https://arxiv.org/abs/2507.09089 - "Measuring the Impact of Early-2025 AI on Experienced Open-Source Developer Productivity"; certainly a case to be made if one includes reviewer productivity)

…redundant test

eerovaher · 2025-12-17T13:57:22Z

docs/changes/nddata/18877.feature.rst

@@ -0,0 +1 @@
+Enhanced error messages in ``extract_array`` using Python 3.11's ``Exception.add_note()`` to provide additional context when ``fill_value`` type is incompatible with the array dtype.


This PR changes some details regarding how exactly the error messages are constructed, but the messages in this PR don't contain any information or provide any additional context compared to the messages in main.

neutrinoceros

I agree with Eero that the changelog fragment is superfluous or misleading. Other than that, I don't think this PR should close the issue it's linked to.
Taking a step back, I also feel like much more work went into reviewing AI output here than would it would normally take to produce the patch manually. This is extractive work at best: extracting value both from human coders from the past whose work was fed into the model, and reviewer time. In my opinion it is not worth its cost, and I would recommend taking collection action against this.

FazeelUsmani added 2 commits November 11, 2025 07:14

Use Exception.add_note for enriched error context

a96e181

Merge branch 'astropy:main' into fix-exceptions-add-note

1bc2f5b

github-actions bot added nddata coordinates labels Nov 11, 2025

FazeelUsmani marked this pull request as draft November 11, 2025 08:13

FazeelUsmani added 5 commits November 11, 2025 13:49

Create 16879.feature.rst

c58d970

Create coordinates/16879.feature.rst

eb85f09

Rename 16879.feature.rst to 18877feature.rst

0df0702

Rename 16879.feature.rst to 18877.feature.rst

cdbf73a

Rename 18877feature.rst to 18877.feature.rst

4ab1f72

FazeelUsmani marked this pull request as ready for review November 11, 2025 09:34

pllim added this to the v8.0.0 milestone Nov 11, 2025

pllim added no-changelog-entry-needed and removed no-changelog-entry-needed labels Nov 11, 2025

FazeelUsmani added 3 commits November 12, 2025 13:19

Merge branch 'main' into fix-exceptions-add-note

d6c6380

Merge branch 'main' into fix-exceptions-add-note

d55fe24

Merge branch 'main' into fix-exceptions-add-note

0299011

neutrinoceros requested changes Nov 12, 2025

View reviewed changes

FazeelUsmani added 4 commits November 12, 2025 19:22

Update test_utils.py

231db9a

Update test_earth.py

1d99e7d

fix ruff format

927c022

fix ruff format

1a5b391

FazeelUsmani requested a review from neutrinoceros November 13, 2025 07:48

FazeelUsmani added 3 commits November 13, 2025 13:19

Merge branch 'main' into fix-exceptions-add-note

0503072

Merge branch 'main' into fix-exceptions-add-note

0e37acd

Merge branch 'main' into fix-exceptions-add-note

66318ea

nstarman reviewed Nov 29, 2025

View reviewed changes

astropy/coordinates/tests/test_earth.py Outdated Show resolved Hide resolved

Update astropy/coordinates/tests/test_earth.py

b693fee

Co-authored-by: Nathaniel Starkman <nstarman@users.noreply.github.com>

FazeelUsmani requested a review from nstarman November 30, 2025 16:52

nstarman approved these changes Nov 30, 2025

View reviewed changes

mwcraig approved these changes Dec 1, 2025

View reviewed changes

eerovaher reviewed Dec 1, 2025

View reviewed changes

eerovaher requested changes Dec 1, 2025

View reviewed changes

FazeelUsmani and others added 2 commits December 2, 2025 15:00

Update astropy/coordinates/tests/test_earth.py

ead0908

Co-authored-by: Eero Vaher <eero.vaher@gmail.com>

Remove changelog entry

d62a348

FazeelUsmani requested a review from eerovaher December 2, 2025 09:42

eerovaher reviewed Dec 2, 2025

View reviewed changes

FazeelUsmani added 2 commits December 12, 2025 16:21

Improve error message for NaN fill_value in extract_array and remove …

855d77b

…redundant test

Remove unused import of Version from test_utils.py

deb3802

FazeelUsmani requested a review from eerovaher December 12, 2025 11:08

mwcraig approved these changes Dec 12, 2025

View reviewed changes

eerovaher reviewed Dec 17, 2025

View reviewed changes

Merge branch 'main' into fix-exceptions-add-note

726d347

nstarman requested a review from eerovaher February 6, 2026 17:12

neutrinoceros requested changes Feb 8, 2026

View reviewed changes

	def test_extract_array_nan_fillvalue():
	if Version(np.__version__) >= Version("1.20"):
	msg = "fill_value cannot be set to np.nan if the input array has"
	with pytest.raises(ValueError, match=msg):
	extract_array(
	np.ones((10, 10), dtype=int), (5, 5), (1, 1), fill_value=np.nan
	)

		@@ -0,0 +1 @@
		Enhanced error messages in ``extract_array`` using Python 3.11's ``Exception.add_note()`` to provide additional context when ``fill_value`` type is incompatible with the array dtype.

Uh oh!

Conversation

FazeelUsmani commented Nov 11, 2025

Changes

Testing

References

Related

Changelog Entry

Uh oh!

github-actions bot commented Nov 11, 2025

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Nov 13, 2025

Uh oh!

FazeelUsmani commented Nov 14, 2025

Uh oh!

FazeelUsmani commented Nov 27, 2025

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

neutrinoceros left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants