Copy input datamodel in source_catalog step

[larrybradley]

With this PR the input datamodel to the source_catalog step is copied before being operated on so that it is left completely unaltered.

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

[schlafly]

When I was thinking about this I imagined only copying model.data (to avoid copying all of the other image planes that aren't really relevant). But I don't know to what extent photutils might touch input data, or if only the model.data -= bkg.background is expected to be relevant.

[larrybradley]

The model.err array is also modified in-place in RomanSourceCatalog to convert from MJy/sr to uJy. But I can can update this to only copy those 2 arrays.

[schlafly]

If the source_catalog code is responsible for all of the modifications and so we can safely save a couple of copies in it and restore them at the end, let's do that. If photutils is "allowed" to modify the arrays we're passing it in the model then let's go ahead and do the full model copy.

Maybe let's also document in the docstring to some function that source_catalog is allowed to change fields data & err by a scale factor (numerical precision differences only). Thanks!

[larrybradley]

The modifications are happening before any inputs to photutils. photutils isn't changing any input values.

[codecov]

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.21%. Comparing base (74f6ab8) to head (196696b).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
romancal/source_catalog/source_catalog_step.py	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1457      +/-   ##
==========================================
- Coverage   76.24%   76.21%   -0.04%     
==========================================
  Files         115      115              
  Lines        7650     7639      -11     
==========================================
- Hits         5833     5822      -11     
  Misses       1817     1817              

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

[schlafly]

Great, then let's try to only save the planes we are explicitly modifying.

[larrybradley]

@schlafly I've updated this PR to copy only the data and err arrays (that are modified in place in the step).

[schlafly]

This looks good to me, pending successful completion of the regression tests.

[schlafly]

Thanks for this test!

[mairanteodoro]

Looks good to me.
I only have one concern about using assert_equal() to compare float32 arrays.

[mairanteodoro]

I think it would be safer to use assert_allclose() because both arrays are of type float32.

[larrybradley]

With this PR the arrays really are identical, so assert_equal should always work.

[schlafly]

This "fails" regtests but I think that's just saying that this is working and it's a numerical noise thing that is now fixed that we should okify. The only failures are pipeline tests that run source cataloging with small numerical differences.
https://github.com/spacetelescope/RegressionTests/actions/runs/11839507319/job/32991232674

[larrybradley]

This was out-of-sync with main, so I just rebased. I'll merge when CI passes again.