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

RCAL-701: Level 3 Mosaic Update #288

Merged
merged 26 commits into from
Feb 1, 2024

Conversation

PaulHuwe
Copy link
Collaborator

@PaulHuwe PaulHuwe commented Nov 2, 2023

Resolves RCAL-701

Closes #

This PR updates the contents of the WFI_Mosaic datamodel, maker utilities, and tests to a more streamlined metadata design for level 3 products.

This update is expected to require new RomanCAL regression test files, so there will be no jenkins link below. There is a RomanCAL ticket for the code dependency changes: spacetelescope/romancal#1057

Checklist

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (9402e53) 97.16% compared to head (a96b3e1) 97.48%.

Files Patch % Lines
src/roman_datamodels/datamodels/_datamodels.py 89.47% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
+ Coverage   97.16%   97.48%   +0.32%     
==========================================
  Files          28       29       +1     
  Lines        2542     2709     +167     
==========================================
+ Hits         2470     2641     +171     
+ Misses         72       68       -4     

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

Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

FYI, this all looks good to me, but let's not merge until seeing what needs to happen in romancal. Thanks!

@PaulHuwe PaulHuwe force-pushed the RCAL-701_L3Metadata branch from 33f798f to 1c9181c Compare January 24, 2024 22:19
@PaulHuwe
Copy link
Collaborator Author

The old dependency failure is expected, due to the astropy Table tag version.

@PaulHuwe PaulHuwe marked this pull request as ready for review January 24, 2024 22:53
@PaulHuwe PaulHuwe requested review from WilliamJamieson and a team as code owners January 24, 2024 22:53
@braingram
Copy link
Collaborator

Any reason to not bump asdf-astropy>=0.5.0 to fix the oldestdeps?

@PaulHuwe PaulHuwe force-pushed the RCAL-701_L3Metadata branch from 5471575 to 58dbabc Compare January 26, 2024 12:31
@PaulHuwe PaulHuwe force-pushed the RCAL-701_L3Metadata branch from 0bc773f to 4347968 Compare January 26, 2024 18:35
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

This looks good to me. I am leery of actually using the append_individual_image_meta stuff until we have actually tried running it on real images; I worry about weird things in the metadata like ndarrays from source detection, etc.. But it's fine for the moment since we haven't actually turned that on in romancal, I think? Can you and @stscieisenhamer coordinate to merge these and get the L3 bits working with the new schema? Thank you!

@PaulHuwe PaulHuwe force-pushed the RCAL-701_L3Metadata branch from 72bd5ec to 853e477 Compare January 28, 2024 19:37
@PaulHuwe PaulHuwe force-pushed the RCAL-701_L3Metadata branch from a01410f to a547611 Compare January 29, 2024 16:25
@PaulHuwe PaulHuwe force-pushed the RCAL-701_L3Metadata branch from e91f1bb to 94e5fba Compare January 29, 2024 17:22
@PaulHuwe PaulHuwe force-pushed the RCAL-701_L3Metadata branch from edf17f9 to d68795a Compare January 29, 2024 19:12
@PaulHuwe PaulHuwe force-pushed the RCAL-701_L3Metadata branch from 1cbecb9 to f57cc4d Compare February 1, 2024 01:53
@PaulHuwe PaulHuwe requested a review from ddavis-stsci February 1, 2024 18:05
Copy link
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

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

LGTM, you don't need to change anything.

roman_datamodels.stnode.MosaicAssociations
"""

mosass = stnode.MosaicAssociations()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just an observation: we usually we abbreviate association as asn so mosasn instead of mosass.
You did get a good laugh out of me.

@PaulHuwe PaulHuwe merged commit 2af32d9 into spacetelescope:main Feb 1, 2024
15 of 16 checks passed
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.

5 participants