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

remove unused V23ToSkyConverter from jwst.transforms extension 1.0.0 #184

Merged
merged 2 commits into from
Jul 19, 2023

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Jul 18, 2023

V23ToSkyConverter is supported in the jwst.transforms asdf extension version 0.7.0 (see 0.7.0 manifest) but is not currently supported in 1.0.0 (see 1.0.0 manifest)

This removes V23ToSkyConverter from the 1.0.0 extension (which already does not support the tag) and in all prior versions of asdf this converter was ignored.

Previously asdf ignored any Converter that lists tags that are not in the manifest. This behavior is undocumented and may conflict with future asdf changes. asdf is changing Converter.select_tag to allow Converter deferral (see asdf-format/asdf#1561) where a converter can support no tags but still support types (where the converter will convert the object being serialized to a new type which is handled by a different converter).

Checklist

  • added entry in CHANGES.rst (either in Bug Fixes or Changes to API)
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)

V23ToSkyConverter is supported in the jwst.transforms asdf extension
version 0.7.0 (see 0.7.0 manifest) but is not currently supported in
1.0.0 (see 1.0.0 manifest)

This removes V23ToSkyConverter from the 1.0.0 extension. Previously
asdf ignored any Converter that lists tags that are not in the
manifest. Newer versions of asdf will produce errors for Converters
that don't support any supported tags (one's listed in the manifest)
and implement a select_tag method (which this one does by inheriting
from Converter).
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (fbd97cc) 64.06% compared to head (a7088d2) 64.04%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
- Coverage   64.06%   64.04%   -0.02%     
==========================================
  Files         101      101              
  Lines        5565     5563       -2     
==========================================
- Hits         3565     3563       -2     
  Misses       2000     2000              
Impacted Files Coverage Δ
src/stdatamodels/jwst/transforms/extensions.py 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@braingram braingram marked this pull request as ready for review July 18, 2023 18:43
@braingram braingram requested a review from a team as a code owner July 18, 2023 18:43
@braingram
Copy link
Collaborator Author

braingram commented Jul 18, 2023

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.

The 0.7 converters are probably not used any more in any data files.
We can probably run a search on crds files and see if there are any left that use 0.7 versions.
Other than that the archive should only have files using 1.0.
Something to discuss with Howard.

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.

2 participants