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

Switch resample to the new drizzle package API #8866

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mcara
Copy link
Member

@mcara mcara commented Oct 8, 2024

This PR switches resample step to the new drizzle API proposed in spacetelescope/drizzle#134

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 update 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

@mcara mcara added the resample label Oct 8, 2024
@mcara mcara self-assigned this Oct 8, 2024
@mcara mcara requested a review from a team as a code owner October 8, 2024 16:32
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 61.79%. Comparing base (8cf4c4b) to head (d288e1a).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
jwst/resample/resample.py 60.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8866   +/-   ##
=======================================
  Coverage   61.79%   61.79%           
=======================================
  Files         377      377           
  Lines       38827    38822    -5     
=======================================
- Hits        23992    23989    -3     
+ Misses      14835    14833    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Thanks Mihai, looks good overall and my comments are minor. It needs a changelog entry. It would also be good to run our regression test suite; do you have permissions and know how to do that?

jwst/resample/gwcs_drizzle.py Outdated Show resolved Hide resolved
jwst/resample/gwcs_drizzle.py Outdated Show resolved Hide resolved
jwst/resample/gwcs_drizzle.py Outdated Show resolved Hide resolved
jwst/resample/gwcs_drizzle.py Outdated Show resolved Hide resolved
@@ -290,7 +290,7 @@ def resample_group(self, input_models, indices):
if isinstance(img, datamodels.SlitModel):
# must call this explicitly to populate area extension
# although the existence of this extension may not be necessary
img.area = img.area
img.area = img.area
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does this line do? does it break anything to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea. It is a strange line but in Python, sometimes things are magical. Seems to be related to SlitModel and also used (set) in the photom step (multi-slit). Maybe @melanieclarke knows more.

jwst/resample/resample_utils.py Outdated Show resolved Hide resolved
jwst/resample/resample_utils.py Outdated Show resolved Hide resolved
jwst/resample/resample_utils.py Show resolved Hide resolved
@@ -22,7 +22,7 @@ dependencies = [
"astropy>=5.3",
"BayesicFitting>=3.0.1",
"crds>=11.17.14",
"drizzle>=1.15.0",
"drizzle @ git+https://github.com/mcara/drizzle.git@redesign-api",
Copy link
Collaborator

Choose a reason for hiding this comment

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

remember to change to @main before merge

@emolter emolter added this to the Build 11.2 milestone Oct 8, 2024
@mcara mcara force-pushed the new-drizzle-api branch 2 times, most recently from 9b25f04 to bcc70b9 Compare October 9, 2024 03:43
@mcara
Copy link
Member Author

mcara commented Oct 9, 2024

grid = gwcs.wcstools.grid_from_bounding_box(bb)
pixmap = np.dstack(reproject(in_wcs, out_wcs)(grid[0], grid[1]))
if shape is not None and not np.array_equiv(shape, in_wcs.array_shape):
in_wcs = deepcopy(in_wcs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is a deepcopy necessary here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure. It felt to be a safer way because this method sets array_shape property of the WCS object. If there is no drawback in modifying inputs, deepcopy can be omitted.

@braingram
Copy link
Collaborator

Is the new drizzle API intended to make direct calls to tdriz tblot unnecessary?

@mcara
Copy link
Member Author

mcara commented Oct 10, 2024

Is the new drizzle API intended to make direct calls to tdriz tblot unnecessary?

In general, new drizzle API is not "intended" to rely on tdriz and such but these calls are available to the developers. That is, the Python API was the one that was modified, not the C-based function API.

The purpose of this PR is to adapt the current code that otherwise would break to the new API when needed. We know that we are going to switch to the new API anyway via other PRs in jwst and stcal when the resample code is migrated there. So this PR simply switches to the new drizzle code with minimal effort in order to understand and OKify (when reasonable) regression test changes due to (minor) bug fixes and the Python class redesign.

CC: @nden

@emolter
Copy link
Collaborator

emolter commented Oct 11, 2024

This looks mostly good to me now, but there are 12 failing tests in the regression test run and only 5 failing tests in last night's nightly run. Do these changes look to you like they are caused by this PR, and are any of them large?

@nden
Copy link
Collaborator

nden commented Oct 13, 2024

@mcara I may be confused about what this PR is meant to accomplish.

  • How is it using the new API in drizzle? The only change here is calling calc_pixmap from drizzle with a modified signature. The rest of the changes are simply reformatting lines (I prefer these not be included) or fixing white spaces.
  • Changing the signature of calc_pixmap means the input WCS needs to be modified to include the change in shape, which means a deepcopy of the WCS is needed. Deep copies of WCS objects are usually expensive and I would prefer the old signature in order to avoid the copies.
  • The new implementation of calc_pixmap computes the input pixels in a different way and does not follow the adopted convention (using grid_from_bounding_box) . This means the inputs are probably slightly off the center of pixels and this will lead to different results. Is there a reason not to use the convention? If you are concerned that gwcs will become a dependency of drizzle then the function should go in stcal (there might even be one there already).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants