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

Update absolute separation default. #1352

Merged

Conversation

schlafly
Copy link
Collaborator

@schlafly schlafly commented Aug 5, 2024

This PR updates the default absolute separation used in tweakreg in romancal, as discussed in #1329.

Closes #1329.

If multiple sources within 'tolerance' can be found, tweakreg may fail. Accordingly, 'separation' should always be larger than 'tolerance.' The default for tweakreg were not consistent with this requirement, which led to a failure to tweak the WCS in some cases. This PR updates the default to avoid this case.

Regression tests fail due to very small differences in the stars that tweakreg selects for matching. This strikes me as okay. There's room in the future to choose better values for these parameters; they should by a few times larger than the expected uncertainty in the pointing. But this resolves the crashes @braingram was seeing.

Checklist

@schlafly schlafly added this to the 24Q4_B15 milestone Aug 5, 2024
@schlafly schlafly requested a review from a team as a code owner August 5, 2024 14:54
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.57%. Comparing base (3189a0a) to head (f180f84).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1352   +/-   ##
=======================================
  Coverage   78.57%   78.57%           
=======================================
  Files         117      117           
  Lines        7861     7861           
=======================================
  Hits         6177     6177           
  Misses       1684     1684           
Flag Coverage Δ *Carryforward flag
nightly 62.26% <ø> (ø) Carriedforward from 3189a0a

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

@schlafly
Copy link
Collaborator Author

schlafly commented Aug 8, 2024

@ddavis-stsci , mind reviewing? I think we want this to go into B15 so that tweakreg doesn't crash on some SCAs.

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

LGTM. Adding a check similar to what was added in spacetelescope/jwst#8476 might make sense as these are user (and pars file) controllable parameters. Having an informative error seems more useful than the step crashing. I don't have any objection to adding this check in a different PR but I suggest we keep the issue open if that's the plan (or make a new one).

@braingram
Copy link
Collaborator

Ignore my comment about adding the check. Once this step uses stcal it will inherit the check:
https://github.com/spacetelescope/stcal/blob/809de2398dc3843821d71fc5cba88ae591a16eb9/src/stcal/tweakreg/tweakreg.py#L129

@schlafly
Copy link
Collaborator Author

schlafly commented Aug 8, 2024

Perfect. Yeah, I agree that this kind of check makes more sense downstream (really in tweakreg itself?) than in romancal / jwst.

@schlafly
Copy link
Collaborator Author

schlafly commented Aug 8, 2024

I okified the regtests and am merging.

@schlafly schlafly merged commit c0e726a into spacetelescope:main Aug 8, 2024
27 checks passed
@schlafly schlafly deleted the update-abs-separation-default branch August 8, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase abs_separation default value in tweakreg
3 participants