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

Use PSF astrometry in tweakreg. #1578

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

schlafly
Copy link
Collaborator

@schlafly schlafly commented Jan 10, 2025

This PR fixes two issues with tweakreg's accuracy. The first is that we at some point unintentionally switched from using the PSF astrometry (x_psf, y_psf) to the segment centroid astrometry (xcentroid, ycentroid) in tweakreg. This leads to much worse astrometric accuracy. The second is that I had left in a fudge factor in the tweakreg regression test, so that we did not notice when this occurred. This PR switches back to using the PSF astrometry and removes the fudge factor.

Regression tests will fail due to changes in the astrometry. Regression tests are here:
https://github.com/spacetelescope/RegressionTests/actions/runs/12713885744

Three failures in steps that use tweakreg. Note that in these cases the median absolute errors decrease and the shifts become closer to zero arcseconds (when the simulated WCS is correct) or to 1" in dec (when the simulated WCS is intentionally offset by 1 arcsecond in ra / dec, in the tweakreg regression test).

We will need to okify these regression differences when merging.

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
    • Regression test: https://github.com/spacetelescope/RegressionTests/actions/runs/12713885744 - [ ] 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

@schlafly schlafly added this to the 25Q2_B17 milestone Jan 10, 2025
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

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

Project coverage is 28.60%. Comparing base (f538ba0) to head (f36a1dd).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
romancal/tweakreg/tweakreg_step.py 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (f538ba0) and HEAD (f36a1dd). Click for more details.

HEAD has 5 uploads less than BASE
Flag BASE (f538ba0) HEAD (f36a1dd)
6 1
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1578       +/-   ##
===========================================
- Coverage   77.94%   28.60%   -49.35%     
===========================================
  Files         115      112        -3     
  Lines        7622     7576       -46     
===========================================
- Hits         5941     2167     -3774     
- Misses       1681     5409     +3728     

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

@schlafly schlafly marked this pull request as ready for review January 10, 2025 18:54
@schlafly schlafly requested a review from a team as a code owner January 10, 2025 18:54
Copy link
Collaborator

@ddavis-stsci ddavis-stsci left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me

Copy link
Collaborator

@mairanteodoro mairanteodoro left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I haven't checked it yet, but don't we have to update the docs too?

@schlafly
Copy link
Collaborator Author

Thanks, yes, I should update this line of the docs.

either ``'x'`` and ``'y'`` or ``'xcentroid'`` and ``'ycentroid'`` columns which

I'll do that this evening and merge.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation regression_testing testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants