Move common resample code to stcal #8986

mcara · 2024-11-27T17:00:02Z

This PR add the common resample code used by both JWST and Roman pipelines to stcal. For now only imaging mode was switched to the new code. This PR supersedes #8695

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

The code in this PR requires the code from spacetelescope/stcal#320

Tasks

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

codecov · 2024-11-27T17:44:54Z

Codecov Report

Attention: Patch coverage is 91.78470% with 29 lines in your changes missing coverage. Please review.

Project coverage is 64.35%. Comparing base (a9ab10a) to head (d156471).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
jwst/resample/resample_spec_step.py	62.50%	9 Missing ⚠️
jwst/resample/resample_spec.py	91.39%	8 Missing ⚠️
jwst/resample/resample.py	96.36%	6 Missing ⚠️
jwst/resample/resample_utils.py	89.74%	4 Missing ⚠️
jwst/resample/resample_step.py	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8986      +/-   ##
==========================================
- Coverage   64.54%   64.35%   -0.20%     
==========================================
  Files         375      375              
  Lines       38737    38638      -99     
==========================================
- Hits        25002    24864     -138     
- Misses      13735    13774      +39

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

braingram

Thanks for putting this together.

I'd like to see the CI run and regression tests for this PR. I added one comment about psutil which should fix one of the issues preventing the CI from running.

Would you update the pyproject.toml to point to the source branch of spacetelescope/stcal#320? That should (hopefully) allow the CI to run so we can see the coverage and test results with this PR.

Also would you queue up a regtest run?

jwst/resample/resample.py

mcara · 2024-11-28T16:35:12Z

Regression tests pass: https://github.com/spacetelescope/RegressionTests/actions/runs/12059744878

emolter · 2024-11-29T15:05:24Z

jwst/resample/resample.py

+                'v3yangle', 'vparity']
+        for key in rm_keys:
+            if key in model.meta.wcsinfo.instance:
+                del model.meta.wcsinfo.instance[key]


 class ResampleData:


I'm a bit confused about why the majority of ResampleData hasn't been removed, as it appears that functionality is mostly what got moved to stcal. What would it take to allow large portions (or all) of this class to be removed?

emolter · 2024-11-29T15:06:37Z

jwst/resample/resample_utils.py

Same question here: weren't a lot of the functions in here moved to stcal, so shouldn't they be removed from here?

mcara · 2024-12-04T14:41:59Z

Initial regression tests that were using the new code only for imaging are: https://github.com/spacetelescope/RegressionTests/actions/runs/12146883313

After switching spectral code to use the new class, here are the results of regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/12155412170

I most failures that I see are unrelated to my code, I think (such as test_miri_image_detector1[ramp], test_miri_image_detector1[rate], ...). The spectral test failures have new header cards and these could be OKified. Spectral tests are easily identifiable by Extra keyword 'NDRIZ' - these are spectral tests with extra meta info (related to resample). Imaging tests should be OK.

emolter

This is looking great Mihai! Thanks for all the hard work. Most of these comments are minor.

It still feels a bit sad to have to convert the datamodels into dictionaries to pass into stcal, but I can't really think of a better way to do it while maintaining the observatory-agnostic nature of stcal. So I guess I'm happy with it. Maybe it's a bit easier on the romancal side when working with asdf files as compared with fits?

emolter · 2024-12-04T22:11:44Z

jwst/outlier_detection/utils.py

    in_memory = not input_models._on_disk
    indices_by_group = list(input_models.group_indices.values())
    ngroups = len(indices_by_group)

+    return_error = return_error and (resamp.compute_err == "driz_err")


Perhaps it would be a good idea to have a warning if return_error is True but the other conditional is not satisfied? Otherwise a user may wonder why the error is not being returned even though they specified it

emolter · 2024-12-04T22:19:13Z

jwst/resample/resample.py

+        input_models : LibModelAccessBase
+            A `LibModelAccessBase`-based object allowing iterating over
+            all contained models of interest.
+


Where is this type defined? Is there any meaningful case here where this would be anything but ModelLibrary? If not, I suggest just making the type ModelLibrary for clarity

emolter · 2024-12-04T22:25:02Z

jwst/resample/resample.py

+
+        return model_dict
+
+    def create_output_jwst_model(self, ref_input_model=None):
        """ Create a new blank model and update it's meta with info from ``ref_input_model``. """


Suggested change

""" Create a new blank model and update it's meta with info from ``ref_input_model``. """

""" Create a new blank model and update its meta with info from ``ref_input_model``. """

emolter · 2024-12-04T22:25:57Z

jwst/resample/resample.py

+        model.meta.photometry.pixelarea_arcsecsq = info_dict["pixelarea_arcsecsq"]
+
+        model.meta.resample.pointings = info_dict["pointings"]
+        # model.meta.resample.n_coadds = info_dict["n_coadds"]


If this is not needed then remove

emolter · 2024-12-04T22:28:31Z

jwst/resample/resample.py

+        # else:
+        #     assign_wcs_util.update_s_region_spectral(self.output_jwst_model)


Suggested change

# else:

# assign_wcs_util.update_s_region_spectral(self.output_jwst_model)

emolter · 2024-12-04T22:35:55Z

jwst/resample/resample.py

@@ -386,390 +625,64 @@ def resample_many_to_many(self, input_models):
            asn_dict = json.loads(asn.dump()[1])  # serializes the asn and converts to dict
            return ModelLibrary(asn_dict, on_disk=True)

-    def resample_many_to_one(self, input_models):
+    def resample_many_to_one(self):


this is great! it's so compact!

emolter · 2024-12-04T22:48:30Z

jwst/resample/resample_spec.py

+    def input_model_to_dict(self, model):
+        model_dict = super().input_model_to_dict(model)
+        return model_dict


Does this do anything different than super().input_model_to_dict()? If not can it be removed?

emolter · 2024-12-04T22:52:00Z

jwst/resample/resample_spec.py

+        # delete these to reduce the number of failures in the regression tests
+        # TODO: this is helpful info that should be stored in products.
+        # del model.meta.resample.pixel_scale_ratio
+        # del model.meta.resample.pixfrac
+        # del model.meta.resample.weight_type
+        # del model.meta.resample.pointings
+        # del model.meta.cal_step.resample


Can you elaborate on why these cause failures? should these be reinstated before merge?

emolter · 2024-12-04T22:55:44Z

jwst/resample/resample_spec.py

+        # del model.meta.resample.pointings
+        # del model.meta.cal_step.resample
+
+    def build_nirspec_output_wcs(self, input_models, refmodel=None,


It looks to me like this never uses self, is that true?

emolter · 2024-12-04T22:58:55Z

jwst/resample/resample_step.py

+        # with result:
+        #     if len(result) == 1:
+        #         model = result.borrow(0)
+        #         result.shelve(model, 0, modify=False)
+        #         return model


Suggested change

# with result:

# if len(result) == 1:

# model = result.borrow(0)

# result.shelve(model, 0, modify=False)

# return model

mcara added the resample label Nov 27, 2024

mcara requested review from braingram, perrygreenfield, nden and emolter November 27, 2024 17:00

mcara self-assigned this Nov 27, 2024

mcara requested a review from a team as a code owner November 27, 2024 17:00

github-actions bot added the testing label Nov 27, 2024

mcara force-pushed the new-resample-cls2 branch from adc3ca4 to c7286ee Compare November 27, 2024 17:21

braingram reviewed Nov 27, 2024

View reviewed changes

jwst/resample/resample.py Outdated Show resolved Hide resolved

github-actions bot added the installation label Nov 27, 2024

emolter reviewed Nov 29, 2024

View reviewed changes

github-actions bot added outlier_detection documentation labels Dec 3, 2024

mcara force-pushed the new-resample-cls2 branch from 3df4da3 to 3553412 Compare December 3, 2024 17:27

mcara marked this pull request as draft December 3, 2024 18:28

mcara added 7 commits December 3, 2024 13:28

Move common resample code to stcal

87559ad

fix style

de78949

add temporary dependency on stcal my fork

7a22979

remove available memory check

1d2108e

add changelog

9b2dee2

update spec code to use new classes

7c090c3

fix test failures and pep8

679aa51

mcara force-pushed the new-resample-cls2 branch from 1db5ba1 to 679aa51 Compare December 3, 2024 18:29

fix diff in s_region for spec

d156471

mcara marked this pull request as ready for review December 4, 2024 14:42

emolter reviewed Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move common resample code to stcal #8986

Move common resample code to stcal #8986

mcara commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

braingram left a comment

mcara commented Nov 28, 2024

emolter Nov 29, 2024

emolter Nov 29, 2024

mcara commented Dec 4, 2024 •

edited

Loading

emolter left a comment

emolter Dec 4, 2024

emolter Dec 4, 2024

emolter Dec 4, 2024

emolter Dec 4, 2024

emolter Dec 4, 2024

emolter Dec 4, 2024

emolter Dec 4, 2024

emolter Dec 4, 2024

emolter Dec 4, 2024

emolter Dec 4, 2024

	""" Create a new blank model and update it's meta with info from ``ref_input_model``. """
	""" Create a new blank model and update its meta with info from ``ref_input_model``. """

		# else:
		# assign_wcs_util.update_s_region_spectral(self.output_jwst_model)

Move common resample code to stcal #8986

Are you sure you want to change the base?

Move common resample code to stcal #8986

Conversation

mcara commented Nov 27, 2024 • edited Loading

Tasks

stage 1

stage 2

stage 3

other

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

braingram left a comment

Choose a reason for hiding this comment

mcara commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcara commented Dec 4, 2024 • edited Loading

emolter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcara commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading

mcara commented Dec 4, 2024 •

edited

Loading