Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adjust tolerances for coron registration #8717

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Aug 21, 2024

This PR attempts to reduce the differences for coron3 results when run on different systems.

The changes are:

  • remove casts from float32 to float64 and back from float64 to float32 in image registration
  • tighten tolerances for leastsq fit in image registration
  • relax tolerances for regtest result comparisons

I ran a (limited) jenkins run:
https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/1666/pipeline
and (limited) github actions run:
https://github.com/spacetelescope/RegressionTests/actions/runs/10511902948/attempts/1

comparisons of the results using the above tolerances pass. However, the regtests will fail comparison against the current truth files. I suggest that we:

  • merge this PR (when approved)
  • okify the regtest results (the jenkins runs should now pass)
  • run the github actions tests

I believe these should pass although it is possible that follow-up changes will be needed as it's difficult to fully test this PR without generating new truth files. If it's preferable I could generate new truth files, put them in a different location on artifactory and run the jenkins and github actions jobs using those new truth files to judge the changes in this PR before the merge.

Full regtests runs for:

Ignoring all the miri_lrs mtimage failures and the miri_image failure (which all look unrelated) the coron3 tests that fail on jenkins also fail on github actions:

  • test_nircam_coron3_sci_exp[002-psfalign] 18 different pixels found (0.00% different).
  • test_nircam_coron3_sci_exp[003-psfalign] 22 different pixels found (0.00% different).
  • test_nircam_coron3_product[i2d] 30 different pixels found (0.02% different).
  • test_miri_coron3_sci_exp[4-psfalign] 74000 different pixels found (0.76% different).
  • test_miri_coron3_sci_exp[4-psfsub] 1813 different pixels found (0.56% different).
  • test_miri_coron3_sci_exp[5-psfalign] 151570 different pixels found (1.57% different).
  • test_miri_coron3_sci_exp[5-psfsub] 2384 different pixels found (0.74% different).
  • test_miri_coron3_product[i2d]40259 different pixels found (46.80% different).

Pulling down these files and comparing them with tolerances matching the ones in this PR show no differences between jenkins and github actions.

Most differences with the truth files are small (<2%) except for coron3_product[i2d] which shows 46.80% different. Looking at the miri i2d file compared to the truth the differences are mostly small
Screenshot 2024-08-22 at 5 48 49 PM
with the largest differences concentrated in the corner of the image (see the few pixels near the upper left):
Screenshot 2024-08-22 at 5 50 21 PM
which corresponds with extreme values present in both the truth:
Screenshot 2024-08-22 at 5 52 40 PM

and jenkins results (with this PR).
Screenshot 2024-08-22 at 5 53 08 PM

Looking at only the central 50-200 x 50-200 pixels the differences are much smaller
Screenshot 2024-08-22 at 5 54 01 PM

but do show some structure.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • All comments are resolved
  • Make sure the JIRA ticket is resolved properly

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.76%. Comparing base (120ee5a) to head (26f40c7).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8717   +/-   ##
=======================================
  Coverage   61.75%   61.76%           
=======================================
  Files         377      377           
  Lines       38749    38743    -6     
=======================================
- Hits        23931    23929    -2     
+ Misses      14818    14814    -4     

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

@braingram braingram marked this pull request as ready for review August 22, 2024 18:24
@braingram braingram requested a review from a team as a code owner August 22, 2024 18:24
@braingram braingram changed the title tighten tolerance of leastsq in coron image registration adjust tolerances for coron registration Aug 22, 2024
@braingram braingram requested a review from tapastro September 11, 2024 17:00
@stscirij
Copy link
Contributor

I wonder if it might be preferable to select an image region for the comparison that excludes the very noisy (and scientifically unimportant) edges, that way we can keep the tolerances lower

@braingram
Copy link
Collaborator Author

I wonder if it might be preferable to select an image region for the comparison that excludes the very noisy (and scientifically unimportant) edges, that way we can keep the tolerances lower

I think there are multiple issues here:

  1. the leastsq parameters are imprecise (hopefully addressed by this PR)
  2. data casting introduces machine differences (hopefully addressed by this PR)
  3. reference files don't appear to match expectations of the algorithm (which this PR doesn't address and I think is most relevant to your comment)

The psfmask documentation describes the SCI extension of the reference file:

The values in the SCI array give the mask values to be applied to the images when computing relative shifts. The mask acts as a weighting function when performing Fourier fits. The values range from zero (full weighting) to one (pixel completely masked out).

However, looking at one of the nircam reference files: https://jwst-crds.stsci.edu/browse/jwst_nircam_psfmask_0212.fits
the mask looks like:
Screenshot 2024-09-17 at 1 36 07 PM
and the algorithm is giving "full weighting" to what you noted are scientifically unimportant parts of the image (the corners). I agree that there are issues here that likely require attention. Although I'm not quite sure how deep the rabbit hole is as I'm also not sure that the documentation agrees with the algorithm and a 0 value in the mask looks to me like it would completely remove the contribution of the corresponding value from the fit (by multiplying the difference by 0).

return ((target - beta * offset) * mask).ravel()

For the above reasons I attempted to avoid algorithmic changes in this PR.

calwebb_coron3
--------------

- Tighten tolerance of psf alignment. [#8717]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tolerances for the fit are tighter. ftol and xtol for leastsq default to 1.49012e-08. This PR tightens them to 1E-15.

The tolerances for the tests are loosened but as that's a test change I didn't note it in the changelog.

@stscirij
Copy link
Contributor

Yes, my mistake. I was thinking of the tolerances for the comparisons

@braingram
Copy link
Collaborator Author

braingram commented Sep 18, 2024

Yes, my mistake. I was thinking of the tolerances for the comparisons

Would it be helpful to reword the changelog? Maybe something like:

Tighten tolerances of leastsq fit for psf alignment and loosen comparison tolerances in unit tests.

@stscirij
Copy link
Contributor

Probably not worth the effort, unless you feel the original entry was inadequate

@braingram braingram merged commit 2d1e350 into spacetelescope:master Sep 18, 2024
25 checks passed
@braingram braingram deleted the coron3_fit branch September 18, 2024 15:04
@braingram
Copy link
Collaborator Author

This will need an okified regtest run. I see @tapastro is running one now and I'll try to queue one up after that finishes.

@braingram
Copy link
Collaborator Author

Regtest run for okifying started here: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST/3038/

@braingram
Copy link
Collaborator Author

The coron3 differences in the above linked run were okified. The unrelated differences were skipped.

@braingram
Copy link
Collaborator Author

braingram commented Sep 19, 2024

coron3 differences are no longer present on github actions regtest run:
https://github.com/spacetelescope/RegressionTests/actions/runs/10932644886

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants