spacetelescope / romancal

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

Fix bug when setting meta.background in SkyMatchStep. #1233

Closed mairanteodoro closed 3 months ago

mairanteodoro commented 4 months ago

Resolves RCAL-837

This PR addresses a bug in SkyMatchStep that was causing meta.background.subtracted to always be set to None instead of True/False, which was preventing ResampleStep from properly using the sky level as determined by SkyMatchStep and set in meta.background.level.

The changes to the code also required some refactoring to the unit tests.

The code changes in this PR are also related to this old issue: https://github.com/spacetelescope/rad/issues/247

Regression tests The only failed test has nothing to do with the changes in this PR. ~https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/764/~ ~https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/770/~ https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/810/

Checklist

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.87%. Comparing base (d10f06b) to head (a8015c0). Report is 195 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1233 +/- ## ======================================= Coverage 78.87% 78.87% ======================================= Files 117 117 Lines 8071 8071 ======================================= Hits 6366 6366 Misses 1705 1705 ``` | [Flag](https://app.codecov.io/gh/spacetelescope/romancal/pull/1233/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/1233/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spacetelescope) | `62.79% <ø> (ø)` | | Carriedforward from [d10f06b](https://app.codecov.io/gh/spacetelescope/romancal/commit/d10f06b232439331681781ba1ef9d05296da574c?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.

mairanteodoro commented 3 months ago

Attaching comparison of the relevant arrays between the output from the regression test against the truth file from Artifactory. Black and white circles indicate the position of the worst absolute and the worst fractional differences, respectively, as reported by the regtest (ran locally, which, by the way, differs from the coordinates reported when ran on Jenkins).

mairanteodoro commented 3 months ago

Looks good. My rule is that I use "is" when I am comparing objects that may be the same object, and == when I am comparing values. In this context I was surprised by using is to compare bools on 336 but otherwise looks good.

Fixed it. Thanks for catching that overlook of mine. ;)

nden commented 3 months ago

@mairanteodoro This is not attached to a milestone. Was this work included in the last release?

mairanteodoro commented 3 months ago

@mairanteodoro This is not attached to a milestone. Was this work included in the last release?

@nden Not yet. We can see it in this comparison between the main branch and the 0.15.1 release branch (commit 4db6c23feac08507dce253e22f951f1c6c213dea): https://github.com/spacetelescope/romancal/compare/0.15.1...main

nden commented 3 months ago

@mairanteodoro OK, I added a milestone.