8000 Use skymatch from stcal by WilliamJamieson · Pull Request #8901 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use skymatch from stcal #8901

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Feb 17, 2025
Merged

Use skymatch from stcal #8901

merged 4 commits into from
Feb 17, 2025

Conversation

WilliamJamieson
Copy link
Collaborator
@WilliamJamieson WilliamJamieson commented Oct 17, 2024 •
edited by melanieclarke
Loading

spacetelescope/stcal#310 moves the shared skymatch code between jwst and romancal into stcal. This PR switches JWST over to using this code rather than having its own copy.

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 up 8000 date 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
Copy link
codecov bot commented Oct 17, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.61%. Comparing base (320636f) to head (5eb2ed5).
Report is 865 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8901      +/-   ##
==========================================
- Coverage   73.71%   73.61%   -0.11%     
==========================================
  Files         372      368       -4     
  Lines       37111    36296     -815     
==========================================
- Hits        27356    26718     -638     
+ Misses       9755     9578     -177

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

@nden
Copy link
Collaborator
nden commented Oct 22, 2024

@WilliamJamieson cube_skymatch will need to be adjusted because it reuses some of the code in skymatch, e.g.
https://github.com/spacetelescope/jwst/blob/main/jwst/cube_skymatch/cube_skymatch_step.py#L25

@WilliamJamieson WilliamJamieson force-pushed the refactor/move_skymatch_to_stcal branch from b03b8a3 to b5019bb Compare October 22, 2024 20:08
@WilliamJamieson WilliamJamieson force-pushed the refactor/move_skymatch_to_stcal branch from b5019bb to 1b375da Compare February 11, 2025 22:18
@emolter emolter mentioned this pull request Feb 11, 2025
Merged
7 tasks
@WilliamJamieson WilliamJamieson force-pushed the refactor/move_skymatch_to_stcal branch from d5c2ef5 to c367037 Compare February 12, 2025 18:01
@WilliamJamieson
Copy link
Collaborator Author

Regression test run: https://github.com/spacetelescope/RegressionTests/actions/runs/13274030261, are these regression test failures related to these changes?

@WilliamJamieson WilliamJamieson marked this pull request as ready for review February 12, 2025 18:03
@WilliamJamieson WilliamJamieson requested review from a team as code owners February 12, 2025 18:03
@emolter
Copy link
Collaborator
emolter commented Feb 12, 2025

are these regression test failures related to these changes?

Unfortunately I believe so. I'm not aware of any recent failures in those tests on main, and it would make sense because the failing tests call calwebb_image3

emolter
emolter reviewed Feb 12, 2025
View reviewed changes
Copy link
Collaborator
@emolter emolter left a comment

Choose a reason for hiding this comment

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

Everything looks good to me except the failing regression tests

@WilliamJamieson WilliamJamieson force-pushed the refactor/move_skymatch_to_stcal branch 2 times, most recently from e5d5c22 to d750767 Compare February 12, 2025 22:46
@WilliamJamieson WilliamJamieson force-pushed the refactor/move_skymatch_to_stcal branch 2 times, most recently from 430ad88 to 5ff9e9b Compare February 14, 2025 17:43
@mcara
Copy link
Member
mcara commented Feb 15, 2025

The code for region.py in gwcs has better accuracy than the code that was used in jwst. @nden reported two failures in regression tests when #8892 was merged: #8892 (comment) I wonder whether those were the same errors that we are seeing now. Unfortunately that regression test in #8892 has expired and we can no longer see it (unless we recreate exactly the same environment).

Looking at the current regression test, there are 7 different pixels that sit away from the edges of input dithers. For all other pixels, the difference between pixel values (current PR vs truth) is smaller than 6e-6. For these 7 pixels, the difference is > 1e-3 up to 0.014. This needs some investigation however, I do not believe this change indicates a problem with this PR. It is just nice to be able to explain it.

@mcara
Copy link
Member
mcara commented Feb 15, 2025

different_pixels

@mcara
Copy link
Member
mcara commented Feb 16, 2025 •
edited
Loading

Upon further analysis, the differences between those 7 pixels in this PR compared to main is due to small differences in computed relative sky values. This results in different (by minuscule amounts) pixel values in sky-subtracted images used as input to drizzle. In the outlier detection step, two pixels in one of the images now have values slightly above the rejection threshold resulting in them being flagged as cosmic rays. This, in turn, significantly modifies input image's weight (for those two pixels) leading to a total of 7 pixels being different in the resampled image. This kind of differences are expected and here it is simply because those two pixels were already very close to being rejected. We do not see any failures in other tests that use skymatch indicating that these differences in flagging cosmic rays are extremely rare. pyregion.py from gwcs, as I mentioned above, has better implementation compared to the one in jwst, and IMO these two tests should be OKified.

@WilliamJamieson
Copy link
Collaborator Author
WilliamJamieson commented Feb 17, 2025 •
edited
Loading

The code for region.py in gwcs has better accuracy than the code that was used in jwst. @nden reported two failures in regression tests when #8892 was merged: #8892 (comment) I wonder whether those were the same errors that we are seeing now. Unfortunately that regression test in #8892 has expired and we can no longer see it (unless we recreate exactly the same environment).

This is exactly the case. The _det function in gwcs.region exactly matches the value returned by np.cross for 2D vectors. Which is the case we are discussing. Thus the regression test failures seen for these changes in Skymatch are simply the pre #8892 regression test values. We never fully explained why the #8892 changes happened, so these changes are probably the right way to go.

EDIT: I ran the failing tests locally with #8892 reverted and compared the output with what this PR produced and confirm that they are the same.

nden
nden approved these changes Feb 17, 2025
View reviewed changes
Copy link
Collaborator
@nden nden left a comment

Choose a reason for hiding this comment

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

LGTM

emolter
emolter approved these changes Feb 17, 2025
View reviewed changes
Copy link
Collaborator
@emolter emolter left a comment

Choose a reason for hiding this comment

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

LGTM now the regression test failures have been tracked down.

@WilliamJamieson
Copy link
Collaborator Author

Thank you all for the reviews and approvals. Once spacetelescope/stcal#310 is merged, I'll re-point the stcal version back to the main stcal branch which will make this PR ready to be merged.

@nden nden requested review from schlafly and ddavis-stsci and removed request for schlafly and ddavis-stsci February 17, 2025 16:42
@WilliamJamieson WilliamJamieson force-pushed the refactor/move_skymatch_to_stcal branch from 5ff9e9b to 5eb2ed5 Compare February 17, 2025 17:04
tapastro
tapastro approved these changes Feb 17, 2025
View reviewed changes
Copy link
Contributor
@tapastro tapastro left a comment

Choose a reason for hiding this comment

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

👍

@tapastro tapastro merged commit ea2d7ef into spacetelescope:main Feb 17, 2025
30 checks passed
@melanieclarke melanieclarke added this to the Build 11.3 milestone Feb 18, 2025
@WilliamJamieson WilliamJamieson deleted the refactor/move_skymatch_to_stcal branch February 18, 2025 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nden nden nden approved these changes

@mcara mcara mcara approved these changes

@tapastro tapastro tapastro approved these changes

@emolter emolter emolter approved these changes

@kmacdonald-stsci kmacdonald-stsci kmacdonald-stsci approved these changes

Assignees
No one assigned
Labels
cube_skymatch documentation installation skymatch
Projects
None yet
Milestone
Build 11.3
Development

Successfully merging this pull request may close these issues.
7 participants
@WilliamJamieson @nden @emolter @mcara @tapastro @kmacdonald-stsci @melanieclarke
0