Skip to content

Add support for varying CRPIX in the VaryingTransform models for Cryo data#501

Merged
Cadair merged 8 commits intoDKISTDC:mainfrom
Cadair:cryo_wcs
Apr 24, 2025
Merged

Add support for varying CRPIX in the VaryingTransform models for Cryo data#501
Cadair merged 8 commits intoDKISTDC:mainfrom
Cadair:cryo_wcs

Conversation

@Cadair Cadair marked this pull request as draft January 21, 2025 15:41
@Cadair Cadair added the Run downstream CI Run's the downstream CI workflow on a PR label Apr 3, 2025
@Cadair Cadair changed the title Initial Work on Cryo-NIRSP gWCS Support Add support for varying CRPIX in the VaryingTransform models for Cryo data Apr 3, 2025
@Cadair Cadair requested a review from Hyzerputt April 3, 2025 15:27
Comment on lines +69 to +71
# for asdf standard < 1.6
if tag.endswith("varying_celestial_transform-1.1.0"):
crpix_key = "crpix"
Copy link
Member Author

Choose a reason for hiding this comment

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

@braingram how terrible is this? lol

The newer tags have a minimum standard version of 1.6, so our oldest supported asdf writes the old tag.

@Cadair Cadair marked this pull request as ready for review April 7, 2025 11:36
@Cadair Cadair requested a review from SolarDrew April 7, 2025 11:36
tags:
# the tag version does not match the schema version
- schema_uri: "asdf://dkist.nso.edu/schemas/varying_celestial_transform-1.2.0"
tag_uri: "asdf://dkist.nso.edu/tags/varying_celestial_transform-1.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason these have different version numbers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Presumably because at some point in the past we bumped the tag and not the schema.

@Cadair
Copy link
Member Author

Cadair commented Apr 15, 2025

@Hyzerputt The downstream tests are failing on this PR, because we raise a deprecation warning on a renamed kwarg. This is an expected failure, but it brings up an interesting point about the workflow for deprecation warnings. They should make the tests fail because they shouldn't pass silently but they also shouldn't break unrelated things. Will having deprecation warnings fail integration tests and other runs which aren't the inventory unit tests?

@Hyzerputt
Copy link

@Hyzerputt The downstream tests are failing on this PR, because we raise a deprecation warning on a renamed kwarg. This is an expected failure, but it brings up an interesting point about the workflow for deprecation warnings. They should make the tests fail because they shouldn't pass silently but they also shouldn't break unrelated things. Will having deprecation warnings fail integration tests and other runs which aren't the inventory unit tests?

Mileage may vary but generally speaking, deprecation warnings don't fail the CI unit or integration tests.

@Cadair Cadair merged commit be2456e into DKISTDC:main Apr 24, 2025
25 of 27 checks passed
@Cadair Cadair deleted the cryo_wcs branch April 24, 2025 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Run downstream CI Run's the downstream CI workflow on a PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants