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

RCAL-895: allow updating source catalog with tweaked WCS when running ELP #1373

Conversation

mairanteodoro
Copy link
Collaborator

@mairanteodoro mairanteodoro commented Aug 20, 2024

Resolves RCAL-895

This PR implements the update of the source catalog coordinates using the improved WCS calculated by TweakRegStep when processing data through the ELP.

When running the ELP, at the end of the TweakRegStep, the cat file produced by SourceCatalogStep (containing all the sources coordinates) will be read in, updated with the new WCS function, and saved back to disk (overwriting the file content).

Regression tests
All tests are passing: https://github.com/spacetelescope/RegressionTests/actions/runs/11034994349

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@mairanteodoro mairanteodoro added this to the 24Q4_B15 milestone Aug 20, 2024
@mairanteodoro mairanteodoro self-assigned this Aug 20, 2024
@mairanteodoro mairanteodoro changed the title Allow updating source catalog with tweaked WCS when running ELP. RCAL-895: allow updating source catalog with tweaked WCS when running ELP Aug 20, 2024
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

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

Project coverage is 78.71%. Comparing base (3107d3d) to head (e19cf1a).

Files with missing lines Patch % Lines
romancal/pipeline/exposure_pipeline.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1373      +/-   ##
==========================================
+ Coverage   78.67%   78.71%   +0.04%     
==========================================
  Files         117      117              
  Lines        7728     7738      +10     
==========================================
+ Hits         6080     6091      +11     
+ Misses       1648     1647       -1     
Flag Coverage Δ *Carryforward flag
nightly 62.26% <ø> (ø) Carriedforward from a6c03ec

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

@mairanteodoro mairanteodoro marked this pull request as ready for review August 20, 2024 20:45
@mairanteodoro mairanteodoro requested a review from a team as a code owner August 20, 2024 20:45
@mairanteodoro mairanteodoro marked this pull request as draft August 21, 2024 13:35
@mairanteodoro mairanteodoro marked this pull request as ready for review August 23, 2024 14:29
romancal/tweakreg/tweakreg_step.py Outdated Show resolved Hide resolved
romancal/tweakreg/tweakreg_step.py Outdated Show resolved Hide resolved
romancal/pipeline/exposure_pipeline.py Show resolved Hide resolved
romancal/tweakreg/tests/test_tweakreg.py Show resolved Hide resolved
romancal/tweakreg/tests/test_tweakreg.py Show resolved Hide resolved
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

Thanks Mairan, looks, good, one comment inline.

romancal/tweakreg/tests/test_tweakreg.py Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@mairanteodoro mairanteodoro requested a review from nden September 17, 2024 01:51
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Sep 17, 2024
@ddavis-stsci ddavis-stsci modified the milestones: 24Q4_B15, 25Q1_B16 Sep 19, 2024
@zacharyburnett zacharyburnett removed their request for review September 24, 2024 13:03
CHANGES.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@schlafly schlafly left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. And it looks like regtests were passing last week; I presume that's still the case?

Maybe just mentioning this aloud for the moment, I originally expected regtests to fail, since we should be changing the values of the catalog entries. But we don't do compare_asdf against the catalog files (reasoning that it's extra hard to guarantee they don't change), and so we don't expect this change to lead to regtest differences.

@mairanteodoro
Copy link
Collaborator Author

Thanks, this looks good to me. And it looks like regtests were passing last week; I presume that's still the case?

Yes, after fixing the "second okify issue" that you mentioned today in the standup meeting, the regtests for this PR are still passing: https://github.com/spacetelescope/RegressionTests/actions/runs/11034994349

@mairanteodoro mairanteodoro enabled auto-merge (rebase) September 25, 2024 18:57
auto-merge was automatically disabled September 26, 2024 13:58

Rebase failed

@mairanteodoro mairanteodoro merged commit e741d19 into spacetelescope:main Sep 26, 2024
27 checks passed
mairanteodoro added a commit to mairanteodoro/romancal that referenced this pull request Oct 7, 2024
… ELP (spacetelescope#1373)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Zach Burnett <zachary.r.burnett@gmail.com>
Co-authored-by: Brett <brettgraham@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file pipeline testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants