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-965 Provide conversion from TVAC/FPS models to ScienceRawModel #455

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

stscieisenhamer
Copy link
Collaborator

@stscieisenhamer stscieisenhamer commented Jan 30, 2025

Resolves RCAL-965

This PR addresses the issue that TVAC/FPS data cannot be run through the pipeline, mostly due to the fact that the TVAC-related datamodels are frozen. A new method, ScienceRawModel.from_tvac_raw is introduced to do the conversion.

Tasks

  • Update or add relevant roman_datamodels tests.
  • Update relevant docstrings and / or docs/ page.
  • Does this PR change any API used downstream? (If not, label with no-changelog-entry-needed.)
News fragment change types:
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

@stscieisenhamer stscieisenhamer requested a review from a team as a code owner January 30, 2025 20:02
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

Attention: Patch coverage is 92.30769% with 5 lines in your changes missing coverage. Please review.

Project coverage is 97.12%. Comparing base (087a60d) to head (fc00d0e).
Report is 100 commits behind head on main.

Files with missing lines Patch % Lines
tests/test_models.py 87.87% 4 Missing ⚠️
src/roman_datamodels/datamodels/_datamodels.py 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
- Coverage   97.56%   97.12%   -0.44%     
==========================================
  Files          30       37       +7     
  Lines        2788     3341     +553     
==========================================
+ Hits         2720     3245     +525     
- Misses         68       96      +28     

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

raw = mk_level1_science_raw(shape=model.shape)

# Define how to recursively copy all attributes.
def node_update(raw, other):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this address the schema differences?

For example, romancal expects meta.guide_star.window_xstart:
https://github.com/spacetelescope/romancal/blob/8422362dc33e0d4b3dc6702274b07c740acda37c/romancal/dq_init/dq_init_step.py#L55 (it's probably a bug that it gets this from the input and not the RampModel but either way).

This is ok for the current guidestar schema:
https://github.com/spacetelescope/rad/blob/50958b8991c38ab0071b8a1e9b2297e329444a1b/src/rad/resources/schemas/guidestar-1.0.0.yaml#L76
but the TVAC guidestar schema doesn't contain that data (it appears to be gw_window_xstart:
https://github.com/spacetelescope/rad/blob/50958b8991c38ab0071b8a1e9b2297e329444a1b/src/rad/resources/schemas/tvac/guidestar-1.0.0.yaml#L57

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good point! Guiding stuff was not such a concern, but will go through and take a closer look. Got any good schema differs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't used any schema differs before but would be curious if you find one you like.

@stscieisenhamer stscieisenhamer requested review from WilliamJamieson and a team January 30, 2025 20:12
@schlafly
Copy link
Collaborator

schlafly commented Feb 3, 2025

Just trying to figure out how this works... if I do strun roman_elp tvac.asdf, when or where does from_tvac_raw get called?

Re schema differences, I think we should try to hit the things that romancal looks for but we don't need to copy everything over just because. The guide window xstart is an interesting case. romancal looks for it, but TVAC doesn't actually use guide windows and so if the code runs I would be fine with it without messing with guide window things.

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.

3 participants