Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix __str__ for wcs with a None transform #489

Merged
merged 2 commits into from
Feb 27, 2024

Conversation

braingram
Copy link
Collaborator

Constructing a WCS without a transform leads to an error for str(wcs):

w = gwcs.WCS(output_frame="icrs")
print(w)
File ~/projects/src/gwcs/gwcs/wcs.py:1347, in WCS.__str__(self)
   1344 for item in self._pipeline[: -1]:
   1345     #model = item[1]
   1346     model = item.transform
-> 1347     if model.name is not None:
   1348         col2.append(model.name)
   1349     else:

AttributeError: 'NoneType' object has no attribute 'name'

This PR updates WCS.__str__ to handle this case so the output is now:

  From   Transform
-------- ---------
detector      None
    icrs      None

The issue was found due to usage of a wcs without a transform in roman_datamodels maker_utils combined with an update to asdf.info that uses the str representation of objects: asdf-format/asdf#1687

This PR includes a test for a wcs without a transform. It appears that WCS.__str__ is otherwise untested so it might be an improvement to add other wcs configurations to the new test.

Copy link

codecov bot commented Feb 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (eb9d316) 87.28% compared to head (f13d75b) 87.28%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #489   +/-   ##
=======================================
  Coverage   87.28%   87.28%           
=======================================
  Files          22       22           
  Lines        3821     3823    +2     
=======================================
+ Hits         3335     3337    +2     
  Misses        486      486           

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

@braingram braingram marked this pull request as ready for review February 3, 2024 14:07
@braingram braingram requested a review from a team as a code owner February 3, 2024 14:07
@braingram braingram requested a review from nden February 20, 2024 17:32
@nden
Copy link
Collaborator

nden commented Feb 20, 2024

Th issue of using a string for the coordinate frame comes back again and again. I was hesitant to keep supporting it but it's too convenient for testing. So I'll approve it.

Copy link
Collaborator

@nden nden left a comment

Choose a reason for hiding this comment

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

Could you add a change log

@braingram
Copy link
Collaborator Author

Thanks for the review. Is the changelog in f13d75b not showing up? I'm seeing it on my end (and appears to be picked up by the CI).

@nden
Copy link
Collaborator

nden commented Feb 27, 2024

Oops, not sure how I missed that. Thanks!

@nden nden merged commit e108dde into spacetelescope:master Feb 27, 2024
23 checks passed
@braingram braingram deleted the str_none_transform branch February 27, 2024 13:32
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.

None yet

2 participants