JP-3789 Add more FITS comparison support for regression tests

[penaguerrero]

Resolves JP-3789

Closes #

This PR addresses the initial changes we would like to implement in FitsDiff.

Tasks

• request a review from someone specific, to avoid making the maintainers review every PR
• add a build milestone, i.e. Build 11.3 (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#>.datamodels.rst
• changes/<PR#>.scripts.rst
• changes/<PR#>.fits_generator.rst
• changes/<PR#>.set_telescope_pointing.rst
• changes/<PR#>.pipeline.rst

stage 1

• changes/<PR#>.group_scale.rst
• changes/<PR#>.dq_init.rst
• changes/<PR#>.emicorr.rst
• changes/<PR#>.saturation.rst
• changes/<PR#>.ipc.rst
• changes/<PR#>.firstframe.rst
• changes/<PR#>.lastframe.rst
• changes/<PR#>.reset.rst
• changes/<PR#>.superbias.rst
• changes/<PR#>.refpix.rst
• changes/<PR#>.linearity.rst
• changes/<PR#>.rscd.rst
• changes/<PR#>.persistence.rst
• changes/<PR#>.dark_current.rst
• changes/<PR#>.charge_migration.rst
• changes/<PR#>.jump.rst
• changes/<PR#>.clean_flicker_noise.rst
• changes/<PR#>.ramp_fitting.rst
• changes/<PR#>.gain_scale.rst

stage 2

• changes/<PR#>.assign_wcs.rst
• changes/<PR#>.badpix_selfcal.rst
• changes/<PR#>.msaflagopen.rst
• changes/<PR#>.nsclean.rst
• changes/<PR#>.imprint.rst
• changes/<PR#>.background.rst
• changes/<PR#>.extract_2d.rst
• changes/<PR#>.master_background.rst
• changes/<PR#>.wavecorr.rst
• changes/<PR#>.srctype.rst
• changes/<PR#>.straylight.rst
• changes/<PR#>.wfss_contam.rst
• changes/<PR#>.flatfield.rst
• changes/<PR#>.fringe.rst
• changes/<PR#>.pathloss.rst
• changes/<PR#>.barshadow.rst
• changes/<PR#>.photom.rst
• changes/<PR#>.pixel_replace.rst
• changes/<PR#>.resample_spec.rst
• changes/<PR#>.residual_fringe.rst
• changes/<PR#>.cube_build.rst
• changes/<PR#>.extract_1d.rst
• changes/<PR#>.resample.rst

stage 3

• changes/<PR#>.assign_mtwcs.rst
• changes/<PR#>.mrs_imatch.rst
• changes/<PR#>.tweakreg.rst
• changes/<PR#>.skymatch.rst
• changes/<PR#>.exp_to_source.rst
• changes/<PR#>.outlier_detection.rst
• changes/<PR#>.tso_photometry.rst
• changes/<PR#>.stack_refs.rst
• changes/<PR#>.align_refs.rst
• changes/<PR#>.klip.rst
• changes/<PR#>.spectral_leak.rst
• changes/<PR#>.source_catalog.rst
• changes/<PR#>.combine_1d.rst
• changes/<PR#>.ami.rst

other

• changes/<PR#>.wfs_combine.rst
• changes/<PR#>.white_light.rst
• changes/<PR#>.cube_skymatch.rst
• changes/<PR#>.engdb_tools.rst
• changes/<PR#>.guider_cds.rst

[codecov]

Codecov Report

Attention: Patch coverage is 6.47709% with 592 lines in your changes missing coverage. Please review.

Project coverage is 76.51%. Comparing base (e2f4801) to head (5fe0138).
Report is 94 commits behind head on main.

Files with missing lines	Patch %	Lines
jwst/regtest/st_fitsdiff.py	6.05%	559 Missing ⚠️
jwst/scripts/stfitsdiff.py	13.15%	33 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9082      +/-   ##
==========================================
+ Coverage   73.67%   76.51%   +2.83%     
==========================================
  Files         369      510     +141     
  Lines       36407    46326    +9919     
==========================================
+ Hits        26824    35445    +8621     
- Misses       9583    10881    +1298     

Flag	Coverage Δ		*Carryforward flag
nightly	77.20% <ø> (?)		Carriedforward from b9ceada

*This pull request uses carry forward flags. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[melanieclarke]

Thanks for working on this! A couple requests:

• It would be helpful to have a command-line script to call the new fitsdiff (st_fitsdiff?), especially for testing the various options.
• In the original ticket, we'd talked about having allowing a separate tolerance for header and data differences. I don't see that here, unless I missed it? I think that would still be helpful.

Also, I started testing by replacing the regular FITSDiff with STFITSDiff in in regtest jwst/regtest/test_nirspec_mos_spec2.py:

#from astropy.io.fits.diff import FITSDiff
from jwst.regtest.st_fitsdiff import STFITSDiff as FITSDiff

This turned up an error for most of the tests:

        stats = {'mean_value_in_a': np.mean(anonan),
                 'mean_value_in_b': np.mean(bnonan),
>                'max_abs_diff': max(values),
                 'min_abs_diff': min(values),
                 'mean_abs_diff': np.mean(values),
                 'std_dev_abs_diff': np.std(values),
                 'max_rel_diff': max(relative_values),
                 'min_rel_diff': min(values / np.abs(bnonan)),
                 'mean_rel_diff': np.mean(relative_values),
                 'std_dev_rel_diff': np.std(relative_values)}
E       ValueError: max() iterable argument is empty

[penaguerrero]

thanks @melanieclarke! I added the script so if you pip install it now the command stfitsdiff should work from the terminal.

[tapastro]

This is looking good, but I echo Melanie's suggestions - in reviewing this PR against the documented astropy classes, it's not clear what changes and edits have been made in the ST versions. In some cases, it appears as though ~150 lines have been copied over, with a small block of new code inserted. If it's not at all possible to inherit and append to the existing method (i.e. TableDataDiff._diff()), we could really use docstrings that describe the changes made (even if the inherit+append is available, more documentation would be helpful!). Otherwise, it will be difficult to disentangle the ST-specific updates made when compared to the existing astropy options.

[penaguerrero]

There were a lot of changes so running another set of reg test: https://github.com/spacetelescope/RegressionTests/actions/runs/13681811542

[melanieclarke]

Thanks for the updates! I glanced through the code again and I'm doing some more testing locally -- a couple more requests below.

Also, I came across an outlier detection output file that differs in the SCI extension by NaN values only, and the DQ extension in multiple places. Astropy fitsdiff appropriately reports differences in SCI, stfitsdiff incorrectly reports no differences. I'll send you the file in a minute.

[melanieclarke]

I realized testing this that these arguments don't have the single letter shortcuts that astropy fitsdiff has. Can you please add them? They are very convenient for saving typing on the command line.

[melanieclarke]

Thanks for adding these, but they don't all match the astropy fitsdiff options, and I think they probably should so we don't confuse folks who are used to working with the astropy tool.

[penaguerrero]

@melanieclarke thank you! I addressed all your comments, can you please take another look and run your local tests?

[melanieclarke]

Testing the same file again, it is now correctly reporting that there are differences in the SCI, ERR, and VAR extensions. Thanks for the fix!

[melanieclarke]

Thanks for all the fixes and updates. I think this is now in a good state to go ahead and merge. We can all test it more locally and hopefully get it incorporated in the regression tests early in the next build, so we have time to catch any small problems.