Skip to content

Fix to_fits signature to correctly raise NotImplementedError#744

Open
karlhillx wants to merge 3 commits into
spacetelescope:mainfrom
karlhillx:fix-to-fits-signature
Open

Fix to_fits signature to correctly raise NotImplementedError#744
karlhillx wants to merge 3 commits into
spacetelescope:mainfrom
karlhillx:fix-to-fits-signature

Conversation

@karlhillx

Copy link
Copy Markdown

Closes #742

Description

When save() calls to_fits(output_path, *args, **kwargs) from the base model, any subclasses that override to_fits without accepting *args, **kwargs will throw a TypeError (e.g., unexpected keyword argument 'overwrite') before the intended NotImplementedError can be raised.

This PR simply updates the signature of to_fits in the affected reference models (in wcs_ref_models.py and tsophot.py) to accept *args and **kwargs, allowing the NotImplementedError to surface correctly with the intended message.

@karlhillx karlhillx requested a review from a team as a code owner May 15, 2026 22:04
@codecov

codecov Bot commented May 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.62%. Comparing base (f75d362) to head (3eb543d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #744      +/-   ##
==========================================
- Coverage   90.75%   90.62%   -0.14%     
==========================================
  Files          99       99              
  Lines        4577     4577              
==========================================
- Hits         4154     4148       -6     
- Misses        423      429       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@emolter emolter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @karlhillx this looks good.

A unit test would be helpful. I know it's redundant that all of these methods have the same override (and there's an open issue about how to clean up this part of the code), so I'm not suggesting unit-testing all 13 instances, although with a parametrized test it may not be that hard. However I think it would be good to directly test this for at least one of the models that inherits from _SimpleModel.

My suggestion would be to put it here: https://github.com/spacetelescope/stdatamodels/blob/main/tests/jwst/datamodels/test_wcs.py

karlhillx and others added 2 commits May 19, 2026 08:39
Test DistortionModel (a _SimpleModel subclass) to confirm that save()
correctly raises NotImplementedError rather than TypeError when
to_fits() is called with the overwrite kwarg from save().

Closes spacetelescope#742, closes spacetelescope#744.
@karlhillx

Copy link
Copy Markdown
Author

Added a test using DistortionModel covering the full call path through save() with a .fits path, which is what triggers the overwrite=True kwarg. Both tests in test_wcs.py pass. Ready for re-review.

fo.validate()


def test_simple_model_to_fits_not_implemented():

@emolter emolter May 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the structure of the test looks good overall, and thanks for adding it. A few small things:

  • we have a fixture tmp_path that will set up a temporary directory for the test without needing to import tempfile and unlink at the end. It would be helpful here, and you'd be able to avoid the try...finally
  • I don't know what warnings you were attempting to ignore here, but we avoid catching and ignoring warnings in tests, as they may hide a real issue. Warnings are upgraded to errors in our testing CI job.
  • I don't believe it's necessary for this test to explicitly call validate()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attempting to_fits on WcsRefModels raises TypeError instead of NotImplementedError

2 participants