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.32689% with 533 lines in your changes missing coverage. Please review.

Project coverage is 77.59%. Comparing base (8191748) to head (9b3143d).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
jwst/regtest/st_fitsdiff.py	6.15%	503 Missing ⚠️
jwst/scripts/stfitsdiff.py	9.09%	30 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9082      +/-   ##
==========================================
+ Coverage   73.77%   77.59%   +3.81%     
==========================================
  Files         372      509     +137     
  Lines       37272    46758    +9486     
==========================================
+ Hits        27497    36280    +8783     
- Misses       9775    10478     +703     

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

*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.

[stscirij]

Do you want to take the absolute values of a and b before taking the difference? This would make 4 .0 and -4.0 equal

[penaguerrero]

Ah! good catch! yeah, nope. Definitely an oops.

[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.

[stscirij]

Latest commit seems to have fixed the issues I was seeing.

Some suggestions for improvements that can be implemented quickly:

1. Report the data types of each array extension for a and b
2. As well as reporting #NAN pixels, also report #nonNaN pixels and #pixels that are zero:

Something like:

a has 1056768 NaN pixels, 2034 non-NaN pixels of which 154 are zero
b has ....

3. Suppress the runtime warning about invalid value encountered in divide by only performing the divide if the denominator is non-zero

4. make sure the min_rel_diff is numeric. If the min relative difference is NaN, probably safe to replace it with 0.0

Otherwise, this looks great. I have some suggestions for some more long-term features, but I don't think they should get in the way of implementing what has already been done.

[penaguerrero]

I have tested the branch locally without a crash. The only thing to do is replace the import for
from jwst.regtest.st_fitsdiff import STFITSDiff as FITSDiff
The failing test that I am running is reporting as expected with our modifications: test_miri_coron3.py

To provide specific tolerances for different extensions, the following dictionary can be added to the fits default arguments dictionary:

extension_tolerances = {
    'default': {'rtol': 1e-5, 'atol': 1e-7}, 
     'primary': {'rtol': 0.0, 'atol': 0.0},
    'headers': {'rtol': 10.0, 'atol': 8.0},
    'sci': {'rtol': 1e-4, 'atol': 1e-5},
    'err': {'rtol': 1e-6, 'atol': 1e-7},
    'VAR_RNOISE': {'rtol': 1e-3, 'atol': 1e-4}
}

fitsdiff_default_kwargs['extension_tolerances'] = extension_tolerances

diff = FITSDiff(f2, f1, **fitsdiff_default_kwargs)

The key default is to provide a default value for all extensions not listed in the dictionary. The key primary will be used only for the main header, and the key headers will be used for all headers of all extensions. The code is not case-sensitive for the keys.

[penaguerrero]

Thanks @stscirij! I added those changes.