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

Move common resample code to stcal #8695

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mcara
Copy link
Member

@mcara mcara commented Aug 8, 2024

This PR add the common resample code used by both JWST and Roman pipelines to stcal. Also, for the first time, this PR adopts the new drizzle API from spacetelescope/drizzle#134 for the resample code used in the pipelines. For now only imaging mode was switched to the new code

This work is related to https://jira.stsci.edu/browse/AL-835

The code in this PR requires the code from spacetelescope/stcal#279 and spacetelescope/drizzle#134 be installed.

At this moment this is a very rough draft for illustration purpose. It should run with default arguments (except input_models and output file name can be specified; everything else is not guaranteed to work). There are no unit/regression tests and documentation may not match the code. Also, for now I kept the old ResampleData code to allow resampling of spectral data to work with the old code.

Example:

from stdatamodels.jwst.datamodels import ModelContainer
from jwst.resample import ResampleStep
from jwst import resample as jr
step = ResampleStep(ModelContainer(['jw01536070001_0210l_00001_nis_cal.fits']))
step.call(f, single=False)

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

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • All comments are resolved
  • Make sure the JIRA ticket is resolved properly

@mcara mcara added the resample label Aug 8, 2024
@mcara mcara self-assigned this Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 4.38356% with 349 lines in your changes missing coverage. Please review.

Project coverage is 55.47%. Comparing base (1561909) to head (f6677e8).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
jwst/resample/resample.py 3.37% 143 Missing ⚠️
jwst/resample/resample_step.py 4.58% 104 Missing ⚠️
jwst/resample/gwcs_drizzle.py 3.22% 90 Missing ⚠️
jwst/resample/resample_utils.py 22.22% 7 Missing ⚠️
jwst/resample/resample_spec_step.py 16.66% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8695      +/-   ##
==========================================
- Coverage   61.92%   55.47%   -6.46%     
==========================================
  Files         376      377       +1     
  Lines       38685    38962     +277     
==========================================
- Hits        23957    21615    -2342     
- Misses      14728    17347    +2619     

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



class OutputTooLargeError(RuntimeError):
"""Raised when the output is too large for in-memory instantiation"""


class ResampleJWSTModelIO(ResampleModelIO):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Methods in resample.py are used in the OutlierDetectionStep, as well as the ResampleSpecStep. Those need to be integrated into this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Methods in resample.py are used in the OutlierDetectionStep, as well as the ResampleSpecStep. Those need to be integrated into this PR

Yes, but this will be much easier to do after your PR that switches the pipeline to using ModelLibrary is merged.

@mcara mcara force-pushed the new-resample-cls branch 2 times, most recently from a738bb6 to 3baaf32 Compare October 2, 2024 04:54
@braingram
Copy link
Collaborator

Thanks for sharing this. I started to look at the changes but haven't made it all the way through yet. I started 2 branches off of yours:
https://github.com/mcara/jwst/compare/new-resample-cls...braingram:jwst:resample_2p0_bjg?expand=1
https://github.com/mcara/stcal/compare/resample-common-code...braingram:stcal:resample_2p0_bjg?expand=1
I wanted to share them sooner (even though they're far from complete). The main changes are to simplify LibModelAccess removing all but the attribute list effectively untangling it from ModelLibrary. I left a number of notes (mostly for reminders for myself). I restored resample_utils from main as it contains a bug in this PR that makes make_output_wcs crash. This restore likely broke something else but as these branches are quite out-of-date it hopefully doesn't interfere with the prototype.

Copy link
Contributor

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

a couple minor comments. Seems to isolate the any dependencies on data models and the like which is very good.


####################################################
# Code below was left for spectral data for now #
####################################################

class ResampleData:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this now is only used for spectral data, to make it clear, rename the class to ResampleSpectrum and all uses of it in the spectral data.

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 would suggest we don't make this change as we may need to deprecate this class first.

jwst/resample/resample.py Outdated Show resolved Hide resolved
@mcara mcara force-pushed the new-resample-cls branch from 9d5d6ff to 32e4594 Compare October 9, 2024 04:02
@mcara
Copy link
Member Author

mcara commented Oct 9, 2024

I will take care of pre-commit tests in a day.

@mcara mcara mentioned this pull request Nov 27, 2024
10 tasks
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