Hackday mteodoro sky variance readnoise #1556

mairanteodoro · 2024-12-09T20:30:17Z

This PR adds a sky variance array to the mosaic file created by resample (i.e. model.var_sky). It also enables building a drizzle image weighted by the inverse of the sky variance when running resample with weight_type="ivsky".

Regression test

3 expected failing tests due to the new attribute var_sky added to the model.
https://github.com/spacetelescope/RegressionTests/actions/runs/12265188362

Tasks

news fragment change types...

changes/<PR#>.general.rst: infrastructure or miscellaneous change
changes/<PR#>.docs.rst
changes/<PR#>.stpipe.rst
changes/<PR#>.associations.rst
changes/<PR#>.scripts.rst
changes/<PR#>.mosaic_pipeline.rst
changes/<PR#>.patch_match.rst

steps

changes/<PR#>.dq_init.rst
changes/<PR#>.saturation.rst
changes/<PR#>.refpix.rst
changes/<PR#>.linearity.rst
changes/<PR#>.dark_current.rst
changes/<PR#>.jump_detection.rst
changes/<PR#>.ramp_fitting.rst
changes/<PR#>.assign_wcs.rst
changes/<PR#>.flatfield.rst
changes/<PR#>.photom.rst
changes/<PR#>.flux.rst
changes/<PR#>.source_detection.rst
changes/<PR#>.tweakreg.rst
changes/<PR#>.skymatch.rst
changes/<PR#>.outlier_detection.rst
changes/<PR#>.resample.rst
changes/<PR#>.source_catalog.rst

codecov · 2024-12-09T20:40:07Z

Codecov Report

Attention: Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.17%. Comparing base (8422362) to head (332943c).

Files with missing lines	Patch %	Lines
romancal/resample/resample_utils.py	81.81%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1556      +/-   ##
==========================================
+ Coverage   78.15%   78.17%   +0.01%     
==========================================
  Files         116      116              
  Lines        7646     7671      +25     
==========================================
+ Hits         5976     5997      +21     
- Misses       1670     1674       +4

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

romancal/resample/resample_utils.py

braingram · 2024-12-12T16:52:20Z

I follow the addition of the new weight type but I'm not sure I see the connection between the ticket and resampling each computed var_sky into a var_sky added to the mosaic model.

If the goal is to save var_sky in the mosaic model would you open a rad and roman_datamodels PRs to update the schema to include var_sky?

docs/roman/resample/arguments.rst

romancal/resample/resample_utils.py

schlafly

I think this looks conceptually right; thanks! Can you put a couple of example files on /grp/roman somewhere, together with an invocation (run strun roman::resample blah.asn --weight_type skyivm ??) so that I can easily poke at some example input and output files?

romancal/resample/resample_utils.py

mairanteodoro · 2024-12-16T20:27:51Z

If the goal is to save var_sky in the mosaic model would you open a rad and roman_datamodels PRs to update the schema to include var_sky?

Yes, that's the idea.

for more information, see https://pre-commit.ci

… present.

docs/roman/resample/arguments.rst

romancal/resample/resample_utils.py

romancal/resample/resample.py

for more information, see https://pre-commit.ci

braingram

Thanks for updating this.

I left a few more comments and questions.

Would you open a RAD PR to add var_sky to the mosaic schema?

braingram · 2025-01-31T13:56:41Z

romancal/resample/tests/test_resample.py

 def test_resampledata_do_drizzle_default_single_exposure_weight_array(
    exposure_1,
    weight_type,
 ):
    """Test that resample methods return non-empty weight arrays."""

+    # adding a few zero flux pixels
+    for i, model in enumerate(exposure_1):
+        model.data[10 + i, 40 - i] = 0


Would you help me understand how setting pixels offset by the index of the exposure in exposure_1 results in the resampled pixel at [10, 40] being equal to 0? Is each exposure offset by 1 pixel in X and 1 in Y?

I just added a random pixel with zero flux. I was not interested in the position of the pixel, just that we did have a pixel with zero flux.

I'm still confused by this. What's the need for a random zero flux pixel per image? If I comment out this code the test doesn't fail.

braingram · 2025-01-31T14:00:49Z

romancal/resample/resample_utils.py

+                ] / img.data[ok_data] * np.median(img.data)
+                img["var_sky"][~ok_data] = img.var_rnoise[~ok_data]
+            except (AttributeError, KeyError, TypeError, ValueError) as e:
+                raise ValueError("Input model contains invalid data array.") from e


Since this raises an exception (if for instance var_rnoise isn't available won't this stop resample from proceeding if ivsky is selected and var_rnoise isn't available? That behavior makes sense to me but is at odds with the documentation (which says thew weight will be 1 for all pixels in this case).

Also I suggest checking for var_rnoise and var_poisson instead of using the try/except (since the try/except will also trigger for other reasons.

braingram · 2025-01-31T14:01:28Z

romancal/resample/resample_utils.py

+        ref_img = input_models.borrow(index=0)
+        input_models.shelve(model=ref_img, index=0)


Suggested change

ref_img = input_models.borrow(index=0)

input_models.shelve(model=ref_img, index=0)

Is ref_img used?

braingram · 2025-01-31T14:05:46Z

romancal/resample/resample_utils.py

+                img["var_sky"] = np.empty_like(img.data)
+                img["var_sky"][ok_data] = img.var_rnoise[ok_data] + img.var_poisson[
+                    ok_data
+                ] / img.data[ok_data] * np.median(img.data)


If data contains a nan won't this cause every var_sky pixel (for each index wither data != 0) to be nan?

braingram · 2025-01-31T14:07:50Z

romancal/resample/resample_utils.py

+        for i, img in enumerate(input_models):
+            try:
+                ok_data = img.data != 0
+                img["var_sky"] = np.empty_like(img.data)


I think this could be simplified by starting with something like:

img["var_sky"] = img.var_rnoise.copy()

since all pixels are either:

copied from var_sky if ~(img.data != 0)

computed below

braingram · 2025-01-31T14:11:43Z

romancal/resample/resample.py

@@ -86,13 +86,15 @@ def __init__(
            )

        self.input_models = input_models
+        # add sky variance array
+        resample_utils.add_var_sky_array(self.input_models)


This will always re-compute var_sky (even if the input contains a var_sky). If that's the goal (always have resample compute var_sky) what about something like:

if weight is ivsky compute var_sky for each model

when resampling var_sky compute it (if it wasn't computed before)

That way for nonivsky weighting resample won't need to make a separate pass through all the models here (in add_var_sky_array).

braingram · 2025-02-03T18:46:22Z

romancal/resample/tests/test_resample.py

 def test_resampledata_do_drizzle_default_single_exposure_weight_array(
    exposure_1,
    weight_type,
 ):
    """Test that resample methods return non-empty weight arrays."""

+    # adding a few zero flux pixels
+    for i, model in enumerate(exposure_1):
+        model.data[10 + i, 40 - i] = 0


I'm still confused by this. What's the need for a random zero flux pixel per image? If I comment out this code the test doesn't fail.

braingram · 2025-02-03T18:48:47Z

romancal/resample/tests/test_resample.py

@@ -712,6 +717,8 @@ def test_resampledata_do_drizzle_default_single_exposure_weight_array(
        many_to_one_model = output_models_many_to_one.borrow(0)
        assert np.any(many_to_one_model.weight > 0)
        assert np.any(many_to_many_model.weight > 0)
+        assert many_to_one_model.data[10, 40] == 0


What is this line testing? The referenced pixel at 10, 40 (seen as the single highlighted pixel in the upper left) falls in the "fill" region of the output image.

mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from ee3f001 to 2d8b4fe Compare December 9, 2024 20:30

github-actions bot added testing regression_testing labels Dec 9, 2024

mairanteodoro added the no-changelog-entry-needed label Dec 9, 2024

mairanteodoro added this to the 25Q1_B16 milestone Dec 11, 2024

mairanteodoro removed the no-changelog-entry-needed label Dec 11, 2024

mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 37cf8b1 to 2448932 Compare December 11, 2024 14:20

github-actions bot added the documentation Improvements or additions to documentation label Dec 11, 2024

braingram reviewed Dec 11, 2024

View reviewed changes

romancal/resample/resample_utils.py Outdated Show resolved Hide resolved

mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 60b0642 to d8dddf9 Compare December 11, 2024 21:50

mairanteodoro marked this pull request as ready for review December 12, 2024 16:03

mairanteodoro requested a review from a team as a code owner December 12, 2024 16:03

mairanteodoro requested review from braingram and schlafly December 12, 2024 16:03

braingram reviewed Dec 12, 2024

View reviewed changes

docs/roman/resample/arguments.rst Outdated Show resolved Hide resolved

romancal/resample/resample_utils.py Show resolved Hide resolved

romancal/resample/resample_utils.py Show resolved Hide resolved

romancal/resample/resample_utils.py Outdated Show resolved Hide resolved

schlafly reviewed Dec 16, 2024

View reviewed changes

romancal/resample/resample_utils.py Outdated Show resolved Hide resolved

mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 192c1cb to 65f7acd Compare December 17, 2024 14:35

mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 2064e73 to af738f3 Compare December 30, 2024 20:06

ddavis-stsci modified the milestones: 25Q1_B16, 25Q2_B17 Jan 15, 2025

mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from f70fc8a to 48ed0fe Compare January 23, 2025 16:01

mairanteodoro and others added 6 commits January 27, 2025 10:27

Rename ResampleData class weight type parameter.

55fc57a

Add var_sky array to mosaic and enable usage in drizzle weight image.

1159562

Add changelog entry.

6d9f437

Update docs and docstring.

d7df709

[pre-commit.ci] auto fixes from pre-commit.com hooks

a3e8634

for more information, see https://pre-commit.ci

Remove unnecessary code.

25e4a16

mairanteodoro added 10 commits January 27, 2025 10:27

Fix terminology in resample docs.

9f6df6f

Update step spec.

d277a66

Add default value for inverse of variance sky if var_sky array is not…

3ba72ff

… present.

Remove unnecessary check.

5900cf0

Prevent zero division issues.

bc62c2b

Add description of calculation of VAR_SKY.

5717c3b

Fix creating base image.

e8925c6

Fix missing stacklevel keyword in call to warnings.warn().

7173bf3

Fix unit test by adding non-zero background level.

f850013

Add var_sky to model error calculation.

fadc18d

mairanteodoro force-pushed the hackday_mteodoro_sky_variance_readnoise branch from 48ed0fe to fadc18d Compare January 27, 2025 15:27

mairanteodoro requested a review from a team as a code owner January 27, 2025 15:27

braingram reviewed Jan 27, 2025

View reviewed changes

docs/roman/resample/arguments.rst Outdated Show resolved Hide resolved

schlafly reviewed Jan 27, 2025

View reviewed changes

romancal/resample/resample_utils.py Outdated Show resolved Hide resolved

romancal/resample/resample_utils.py Outdated Show resolved Hide resolved

braingram reviewed Jan 27, 2025

View reviewed changes

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

mairanteodoro and others added 6 commits January 29, 2025 15:53

Merge branch 'main' into hackday_mteodoro_sky_variance_readnoise

047350b

Address review comments.

c448778

Fix check style issue.

7c3a31c

Improve docs and logs.

fcb2c17

[pre-commit.ci] auto fixes from pre-commit.com hooks

6507bfd

for more information, see https://pre-commit.ci

Remove escape characters.

332943c

mairanteodoro requested a review from braingram January 30, 2025 15:23

braingram reviewed Jan 31, 2025

View reviewed changes

braingram reviewed Feb 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hackday mteodoro sky variance readnoise #1556

Hackday mteodoro sky variance readnoise #1556

mairanteodoro commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

braingram commented Dec 12, 2024

schlafly left a comment

mairanteodoro commented Dec 16, 2024

braingram left a comment

braingram Jan 31, 2025

mairanteodoro Feb 3, 2025

braingram Feb 3, 2025

braingram Jan 31, 2025

braingram Jan 31, 2025

braingram Jan 31, 2025

braingram Jan 31, 2025

braingram Jan 31, 2025

braingram Feb 3, 2025

braingram Feb 3, 2025

		ref_img = input_models.borrow(index=0)
		input_models.shelve(model=ref_img, index=0)

Hackday mteodoro sky variance readnoise #1556

Are you sure you want to change the base?

Hackday mteodoro sky variance readnoise #1556

Conversation

mairanteodoro commented Dec 9, 2024 • edited Loading

Regression test

Tasks

steps

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

braingram commented Dec 12, 2024

schlafly left a comment

Choose a reason for hiding this comment

mairanteodoro commented Dec 16, 2024

braingram 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

mairanteodoro commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading