spacetelescope / jwst

Python library for science observations from the James Webb Space Telescope
https://jwst-pipeline.readthedocs.io/en/latest/
Other
561 stars 167 forks source link

JP-3668: Move tweakreg into stcal #8624

Closed emolter closed 2 months ago

emolter commented 3 months ago

Resolves JP-3668

Closes #8594

The portions of the tweakreg step that are shared between jwst and romancal are being moved into the stcal repository; see this stcal PR. This PR updates the jwst pipeline accordingly.

Checklist for PR authors (skip items if you don't have permissions or they are not applicable)

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 79.62963% with 11 lines in your changes missing coverage. Please review.

Project coverage is 60.05%. Comparing base (ebd929c) to head (94ce74e). Report is 1 commits behind head on master.

Files Patch % Lines
jwst/resample/resample_utils.py 33.33% 6 Missing :warning:
jwst/tweakreg/tweakreg_step.py 84.84% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #8624 +/- ## ========================================== - Coverage 60.16% 60.05% -0.12% ========================================== Files 370 369 -1 Lines 38666 38409 -257 ========================================== - Hits 23264 23065 -199 + Misses 15402 15344 -58 ```

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

emolter commented 2 months ago

regression tests started here. Still can't figure out why the singular unit test is failing, but if regtests turn up more failures that might give more information.

emolter commented 2 months ago

one-liner change to make the version of update_s_region_imaging in stcal identical to what used to be in the jwst repo did indeed fix the regtest failures. Unfortunately, these seem unrelated to the single unit test failure tweakreg/tests/test_multichip_jwst.py:test_multichip_alignment_step. Still tracking that one down.

emolter commented 2 months ago

@mcara RE the one unit test that is failing: I still don't quite understand what it is trying to do, exactly, but I put the results of some initial exploration of what could be going wrong into a docstring on the test itself. I'm not asking you to diagnose this in detail for me, necessarily, but based on those notes perhaps you can suggest where things might be going wrong or what else I should check.

emolter commented 2 months ago

started new regression test run here

emolter commented 2 months ago

new regression tests started here, after minor changes to both stcal and jwst sides.

edit: lots of failing regression tests due to very small differences in the S_REGION. looking into the reason

emolter commented 2 months ago

Another new set of regtests here

emolter commented 2 months ago

ok, hopefully the last round - I realized I had inadvertently changed the default value of the center parameter in the update_s_region_imaging() function that has now been renamed to compute_s_region_imaging() and moved to stcal. Regression tests started here. The offenders from the last run were passing locally, at least.

emolter commented 2 months ago

regression test run covering fix of the final few failing S_REGION issues - all passing

emolter commented 2 months ago

I'll defer to all the expert reviews already done for this change. From a cursory review, this looks ready to go to me.

It looks like pyproject.toml needs an update after the stcal change is merged, so I'll hold off on approval, pending that coordination.

I just merged the stcal side since Nadia gave the approval. The correct thing to pin is stcal@main, right? And wait for an stcal release to change it to 1.8.0 later? @melanieclarke

melanieclarke commented 2 months ago

I just merged the stcal side since Nadia gave the approval. The correct thing to pin is stcal@main, right? And wait for an stcal release to change it to 1.8.0 later? @melanieclarke

Sounds right to me. @braingram?