RCAL-965 Provide conversion from TVAC/FPS models to ScienceRawModel #455

stscieisenhamer · 2025-01-30T20:02:55Z

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.)
- Write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types).
- Start a romancal regression test (https://github.com/spacetelescope/RegressionTests/actions/workflows/romancal.yml) with this branch installed ("git+https://github.com/<fork>/rad@<branch>").

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

codecov · 2025-01-30T20:06:00Z

Codecov Report

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

Project coverage is 97.19%. Comparing base (087a60d) to head (909c3cf).
Report is 102 commits behind head on main.

Files with missing lines	Patch %	Lines
tests/test_models.py	91.11%	4 Missing ⚠️
src/roman_datamodels/datamodels/_utils.py	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #455      +/-   ##
==========================================
- Coverage   97.56%   97.19%   -0.37%     
==========================================
  Files          30       37       +7     
  Lines        2788     3354     +566     
==========================================
+ Hits         2720     3260     +540     
- Misses         68       94      +26

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

src/roman_datamodels/datamodels/_datamodels.py

schlafly · 2025-02-03T17:02:01Z

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.

stscieisenhamer · 2025-02-05T03:57:20Z

@schlafly This is all invoked in the dq_init_step. See romancal PR#1596

stscieisenhamer · 2025-02-05T14:15:49Z

initial regression run

PaulHuwe

I am a little surprised more changes aren't needed. I notice that there are regression test failures - are these expected?

PaulHuwe · 2025-02-05T23:35:19Z

src/roman_datamodels/datamodels/_datamodels.py

+        # Define how to recursively copy all attributes.
+        def node_update(raw, other):
+            """Implement update to directly access each value"""
+            for key in other.keys():


This is rather similar to:
https://github.com/spacetelescope/roman_datamodels/blob/main/src/roman_datamodels/datamodels/_datamodels.py#L177

and I seem to recall other spots in the code doing similar recursive updating. Maybe it is best to make a utility function that these methods can call?

Agreed. There is a subtle difference now, but will look for a way to handle more generically.

And methinks have accomplished it.

schlafly · 2025-02-06T14:38:35Z

Thanks, I think this looks reasonable. Can you add some documentation of what exactly we're trying to accomplish here, though? Probably as a docstring in from_tvac_raw, but potentially also in the RTD if there's a reasonable place.

i.e., something like:
romancal supports processing a selection of files which use an outdated schema. It supports these with a bespoke method that converts the files to the new format when they are read in dq_init. This conversion does not do a detailed mapping between all of the new and old metadata, but instead opportunistically looks for fields with common names and assigns them. Other metadata with non-matching names is simply copied in place. This allows processing to proceed and preserves the original metadata, but the resulting files have duplicates of many entries.
If that sounds right?

I think the biggest practical issue is that a lot of the guide window stuff changed name and we'll now have those bogus fields twice, which is ugly but practical. That said, since guide windows aren't actually being used for TVAC---I think? @tddesjardins ? --- it's hard for me to justify writing code to do the conversion of all of the unused fields.

Have you reviewed the schema to see if that's the biggest area where things got renamed? I went through Tyler's L2 schema doc to try to make sure, and I think that's the case, but it would be good to have another set of eyes on it.

for more information, see https://pre-commit.ci

stscieisenhamer · 2025-02-09T13:47:37Z

@schlafly I've updated the docstrings in both the roman_datamodel's api and the romancal high-level description of the dq_init step.

stscieisenhamer · 2025-02-09T13:50:20Z

For all reviewers: I believe all the comments have been addressed. Ready for full review

for more information, see https://pre-commit.ci

PaulHuwe · 2025-02-12T03:40:48Z

src/roman_datamodels/datamodels/_utils.py

@@ -23,6 +24,78 @@ class FilenameMismatchWarning(UserWarning):
    """


+def _node_update(to_node, from_node, extras=None, extras_key=None, ignore=None):
+    """Copy node contents from an existing node to an existing node


Suggested change

"""Copy node contents from an existing node to an existing node

"""Copy node contents from an existing node to another existing node

PaulHuwe · 2025-02-12T03:41:17Z

src/roman_datamodels/datamodels/_utils.py

+
+    How the copy occurs depends on existence of keys in `to_node`
+
+    If key exists in `to_node`, contents is converted from `from_node` stnode type to


Suggested change

If key exists in `to_node`, contents is converted from `from_node` stnode type to

If key exists in `to_node`, contents are converted from `from_node` stnode type to

PaulHuwe

LGTM outside of the one question I posed

PaulHuwe · 2025-02-12T03:48:10Z

src/roman_datamodels/datamodels/_utils.py

+            continue
+        if key in to_node:
+            if isinstance(to_node[key], Mapping):
+                _node_update(getattr(to_node, key), getattr(from_node, key), extras=extras, extras_key=extras_key)


Will this potentially cause a value to accidentally move things to extras if, say, a sub-keyword happens to have the same name as a root level keyword that is in extras? Since the extra block isn't examining the hierarchy feeding it?

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

stscieisenhamer requested review from PaulHuwe and ddavis-stsci January 30, 2025 20:08

braingram reviewed Jan 30, 2025

View reviewed changes

src/roman_datamodels/datamodels/_datamodels.py Outdated Show resolved Hide resolved

stscieisenhamer requested review from WilliamJamieson and a team January 30, 2025 20:12

stscieisenhamer force-pushed the rcal-965-tvac branch from fc00d0e to c89df59 Compare February 5, 2025 04:00

PaulHuwe requested changes Feb 5, 2025

View reviewed changes

stscieisenhamer and others added 6 commits February 7, 2025 13:14

fix RampModel.from_science_raw to handle TvacModel

c50698c

add converter from_tvac_raw

c7809f4

update conversion tests to use flat dicts

7fdb191

add test for from_tvac_raw converter

b18d2ee

[pre-commit.ci] auto fixes from pre-commit.com hooks

cabf0f0

for more information, see https://pre-commit.ci

update changelog

1f4a703

stscieisenhamer force-pushed the rcal-965-tvac branch from c89df59 to 1f4a703 Compare February 7, 2025 18:14

stscieisenhamer and others added 6 commits February 8, 2025 08:33

Move colliding meta attributes into extras.tvac

4af5fbb

[pre-commit.ci] auto fixes from pre-commit.com hooks

bd8b6e1

for more information, see https://pre-commit.ci

create common utility to update nodes

851817e

update ScienceRawModel to use new utility

2b719ef

update RampModel to use conversion utility

131995f

[pre-commit.ci] auto fixes from pre-commit.com hooks

965b537

for more information, see https://pre-commit.ci

stscieisenhamer added 2 commits February 10, 2025 10:48

update docstrings

f3933e6

add test for invalid input

8c2d151

stscieisenhamer and others added 4 commits February 11, 2025 15:15

update docstring

e80629b

test for resultantdq and ignore node copying

0c5fc6a

add test for extras

5929515

[pre-commit.ci] auto fixes from pre-commit.com hooks

909c3cf

for more information, see https://pre-commit.ci

stscieisenhamer requested review from braingram and schlafly February 11, 2025 21:10

PaulHuwe reviewed Feb 12, 2025

View reviewed changes

PaulHuwe requested changes Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCAL-965 Provide conversion from TVAC/FPS models to ScienceRawModel #455

RCAL-965 Provide conversion from TVAC/FPS models to ScienceRawModel #455

stscieisenhamer commented Jan 30, 2025 •

edited

Loading

codecov bot commented Jan 30, 2025 •

edited

Loading

schlafly commented Feb 3, 2025

stscieisenhamer commented Feb 5, 2025

stscieisenhamer commented Feb 5, 2025

PaulHuwe left a comment

PaulHuwe Feb 5, 2025

stscieisenhamer Feb 7, 2025

stscieisenhamer Feb 9, 2025

schlafly commented Feb 6, 2025

stscieisenhamer commented Feb 9, 2025

stscieisenhamer commented Feb 9, 2025

PaulHuwe Feb 12, 2025

PaulHuwe Feb 12, 2025

PaulHuwe left a comment

PaulHuwe Feb 12, 2025

	"""Copy node contents from an existing node to an existing node
	"""Copy node contents from an existing node to another existing node


		How the copy occurs depends on existence of keys in `to_node`

		If key exists in `to_node`, contents is converted from `from_node` stnode type to

	If key exists in `to_node`, contents is converted from `from_node` stnode type to
	If key exists in `to_node`, contents are converted from `from_node` stnode type to

RCAL-965 Provide conversion from TVAC/FPS models to ScienceRawModel #455

Are you sure you want to change the base?

RCAL-965 Provide conversion from TVAC/FPS models to ScienceRawModel #455

Conversation

stscieisenhamer commented Jan 30, 2025 • edited Loading

Tasks

codecov bot commented Jan 30, 2025 • edited Loading

Codecov Report

schlafly commented Feb 3, 2025

stscieisenhamer commented Feb 5, 2025

stscieisenhamer commented Feb 5, 2025

PaulHuwe left a comment

Choose a reason for hiding this comment

PaulHuwe Feb 5, 2025

Choose a reason for hiding this comment

stscieisenhamer Feb 7, 2025

Choose a reason for hiding this comment

stscieisenhamer Feb 9, 2025

Choose a reason for hiding this comment

schlafly commented Feb 6, 2025

stscieisenhamer commented Feb 9, 2025

stscieisenhamer commented Feb 9, 2025

PaulHuwe Feb 12, 2025

Choose a reason for hiding this comment

PaulHuwe Feb 12, 2025

Choose a reason for hiding this comment

PaulHuwe left a comment

Choose a reason for hiding this comment

PaulHuwe Feb 12, 2025

Choose a reason for hiding this comment

stscieisenhamer commented Jan 30, 2025 •

edited

Loading

codecov bot commented Jan 30, 2025 •

edited

Loading