RCAL-930 Roundtrip L3 wcsinfo especially when skycell specifications are used

[stscieisenhamer]

Resolves RCAL-930

Closes #1431

This PR addresses ensures that the L3 meta.wcsinfo can be used to accurately reproduce the originating WCS from which the wcsinfo block had been derived. In particular, when the L3 product has been created based on a skycell instead of the input images, this update ensures that roundtripping of the wcsinfo is still valid.

Tasks

• request a review from someone specific, to avoid making the maintainers review every PR
• add a build milestone, i.e. 24Q4_B15 (use the latest build if not sure)
• Does this PR change user-facing code / API? (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)
  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • start a regression test and include a link to the running job (click here for instructions)
    • Do truth files need to be updated ("okified")?
      • after the reviewer has approved these changes, run okify_regtests to update the truth files
• if a JIRA ticket exists, make sure it is resolved properly

news fragment change types...

• changes/<PR#>.general.rst: infrastructure or miscellaneous change
• changes/<PR#>.docs.rst
• changes/<PR#>.stpipe.rst
• changes/<PR#>.associations.rst
• changes/<PR#>.scripts.rst
• changes/<PR#>.mosaic_pipeline.rst
• changes/<PR#>.patch_match.rst

steps

• changes/<PR#>.dq_init.rst
• changes/<PR#>.saturation.rst
• changes/<PR#>.refpix.rst
• changes/<PR#>.linearity.rst
• changes/<PR#>.dark_current.rst
• changes/<PR#>.jump_detection.rst
• changes/<PR#>.ramp_fitting.rst
• changes/<PR#>.assign_wcs.rst
• changes/<PR#>.flatfield.rst
• changes/<PR#>.photom.rst
• changes/<PR#>.flux.rst
• changes/<PR#>.source_detection.rst
• changes/<PR#>.tweakreg.rst
• changes/<PR#>.skymatch.rst
• changes/<PR#>.outlier_detection.rst
• changes/<PR#>.resample.rst
• changes/<PR#>.source_catalog.rst

[stscieisenhamer]

Regression test 1

Result: Only one item that is expected and will need to be okified.

[codecov]

Codecov Report

Attention: Patch coverage is 84.78261% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.15%. Comparing base (cb24749) to head (6ebb223).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
romancal/regtest/util.py	33.33%	6 Missing ⚠️
romancal/pipeline/mosaic_pipeline.py	96.42%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1585      +/-   ##
==========================================
+ Coverage   77.92%   78.15%   +0.23%     
==========================================
  Files         115      116       +1     
  Lines        7619     7646      +27     
==========================================
+ Hits         5937     5976      +39     
+ Misses       1682     1670      -12     

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

[schlafly]

This looks good. My chief concern is whether we're populating the "real" L3 WCS correctly, though, and I'm not 100% sure that this captures that case? I would suggest one of the following two tests:

• regression test comparing wcsinfo_to_wcs of one of the resampled L3 files in the regression tests to l3.meta.wcs
• unit test comparing something like

wcs1 = skycell_to_wcs(skycell_record)
l3 = dummy_l3()
gwcs_into_l3(l3, wcs1)
wcs2 = wcsinfo_to_wcs(l3.meta.wcsinfo)
# compare wcs1, wcs2

I'm also interested in knowing which of these two cases
https://github.com/spacetelescope/romancal/pull/1585/files#diff-15cbca0fd660d7d5eac167ece3633d2a1a665d776bf180b07617f06b5fb40a7bR229-R231
is the usual branch we take for normal files we produce; I hope that it's the one on line 230 and not the one on line 232-236?

Did you end up being able to reproduce the WCS nearly perfectly?

[stscieisenhamer]

I have pushed the wcs comparison tests. The dec differences are < 1.e-5 arcsec. The ra differences are < 5e-4 arcsec. If one looks at the wcs info itself, in particular the scaling, they are similar to the 6th decimal place. Without making the wcs to wcsinfo func very knowledgeable about the exact info in a wcs (right now it is fairly general), this is the best, due to calculation roundoff.

Concerning the question of whether the matrix is used directly or not. Methinks this is the wrong question to be asking. There is really only one situation where this routine would normally be called by MosaicPipeline and that is when a skycell has been defined. As best as I can tell, the patches file does not define a rotation matrix, but only "orientation". I have also not done a search, but I presume most skycells are not defined with 0 orientation anyways. Hence, it is mute.

Otherwise, the wcsinfo_to_wcs is just a convenience function. If MosaicPipeline does not have a skycell, it will be computing the wcs from the input images' wcs' anyways.

[schlafly]

Thanks. It looks to me like the mad_std are actually 10^-5 mas, rather than arcseconds, which is negligible. The biggest difference in the regtest I looked at was the median overall offset rather than the standard deviation, coming from the scale factor times the distance from the center. This was ~3e-3 mas, which is small but not utterly negligible. If I artificially forced the scale factor to match exactly, this all went away and things agreed perfectly.

I think we have two options if we want to get that extra precision:

1. we can explicitly copy over factor_3 and factor_4 kind of as we do here
   https://github.com/spacetelescope/romancal/blob/main/romancal/resample/resample.py#L835-L836
   That's fragile but will work for our usual skycells and so is maybe okay.
2. we can try changing https://github.com/spacetelescope/stcal/blob/main/src/stcal/alignment/util.py#L361 to have a parameter that lets one specify a larger than one pixel step, so that we get less numerical error there.

Do you have a sense here? I'm okay with either of those options, but do think we should try to get the median shift down to the <1 uas range.

[schlafly]

Looks great, thanks!

[stscieisenhamer]

For the record, here are some plots of the footprints of a few adjacent skycells and all skycells.