spacetelescope / romancal

Python library to process science observations from the Nancy Grace Roman Space Telescope
https://roman-pipeline.readthedocs.io/en/latest/
Other
28 stars 28 forks source link

RCAL-830: update tweakreg to use source catalog #1276

Open mairanteodoro opened 2 weeks ago

mairanteodoro commented 2 weeks ago

Resolves RCAL-830

This PR addresses the integration between TweakRegStep and SourceCatalogStep.

Regression tests

Checklist

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 85.71429% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79.36%. Comparing base (74a137c) to head (f6f2364).

Files Patch % Lines
romancal/tweakreg/tweakreg_step.py 75.00% 4 Missing :warning:
romancal/pipeline/exposure_pipeline.py 33.33% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1276 +/- ## ========================================== - Coverage 79.39% 79.36% -0.04% ========================================== Files 117 117 Lines 8076 8097 +21 ========================================== + Hits 6412 6426 +14 - Misses 1664 1671 +7 ``` | [Flag](https://app.codecov.io/gh/spacetelescope/romancal/pull/1276/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | Coverage Δ | | *Carryforward flag | |---|---|---|---| | [nightly](https://app.codecov.io/gh/spacetelescope/romancal/pull/1276/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | `62.83% <ø> (-0.02%)` | :arrow_down: | Carriedforward from [48a2117](https://app.codecov.io/gh/spacetelescope/romancal/commit/48a211797f33213d0b3cf171227c227189032b60?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | *This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

schlafly commented 1 day ago

This looks reasonable to me. Can you confirm that you can run the ELP successfully and that regtests pass, except for compare_asdf failures having to do with missing source_detection stuff and slight changes to the WCS?

I think @braingram's suggestion was to remove return_updated_model from the spec and then replace this line with a getattr() call that uses False as a default. https://github.com/spacetelescope/romancal/pull/1276/files#diff-20b0d7c2e6ef2cb1dfd2df9d42ecdabdd86fb400b73755cb4a58598ddd200c2eR133 I think that looks straightforward, but you know better.

mairanteodoro commented 1 day ago

This looks reasonable to me. Can you confirm that you can run the ELP successfully and that regtests pass, except for compare_asdf failures having to do with missing source_detection stuff and slight changes to the WCS?

Yes, I can confirm that we can run the ELP successfully. These are the results from an association file containing 5 files (the default behavior for the ELP is to save the cat and segm files in addition to the updated model):

Screenshot 2024-07-03 at 12 18 35 PM

The failing regression tests are all reporting expected differences between the new output and the previous truth files. The GA output can be seen here: https://github.com/spacetelescope/RegressionTests/actions/runs/9780961333

mairanteodoro commented 1 day ago

I think these changes would allow removal of the option from the spec. I think of the spec as part of the user-facing API and as @schlafly noted the "return_updated_model" is only something the pipeline is concerned with.

Thanks, @braingram!