Initial sky matching implementation #687

bmorris3 · 2023-04-26T13:52:05Z

This PR provides an initial adaptation of the skymatch module in jwst implemented by @mcara. I have made every effort to preserve the same functionality, tests, and docs as jwst.skymatch.

The primary difference between the implementations is that I have (temporarily?) dropped support for associations, since the definition of associations for JWST and Roman is likely to be different in important ways that are still partly TBD.

Locally the tests pass and the docs build successfully only when using roman_datamodels with the fix in: spacetelescope/roman_datamodels#164

The algorithm is summarized nicely in Makovoz et al. (2005). In short, it takes a group of images with different background levels (see the left panel below) and solves a system of equations for the best sky background level in each image to bring all backgrounds into agreement. The resulting matched and subtracted images are shown in the right panel below.

Checklist

added entry in CHANGES.rst under the corresponding subsection
updated relevant tests
updated relevant documentation
updated relevant milestone(s)
added relevant label(s)

codecov · 2023-04-26T14:04:58Z

Codecov Report

Patch coverage: 64.34% and project coverage change: +10.59 🎉

Comparison is base (67639b2) 64.55% compared to head (2684287) 75.14%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #687       +/-   ##
===========================================
+ Coverage   64.55%   75.14%   +10.59%     
===========================================
  Files          78       88       +10     
  Lines        3950     5243     +1293     
===========================================
+ Hits         2550     3940     +1390     
+ Misses       1400     1303       -97

Flag	Coverage Δ		*Carryforward flag
nightly	`64.55% <ø> (ø)`		Carriedforward from 2efec43

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
romancal/lib/suffix.py	`94.28% <ø> (ø)`
romancal/skymatch/skyimage.py	`47.07% <47.07%> (ø)`
romancal/skymatch/skymatch_step.py	`74.35% <74.35%> (ø)`
romancal/skymatch/skymatch.py	`80.22% <80.22%> (ø)`
romancal/skymatch/region.py	`80.33% <80.33%> (ø)`
romancal/skymatch/skystatistics.py	`92.10% <92.10%> (ø)`
romancal/skymatch/__init__.py	`100.00% <100.00%> (ø)`

... and 29 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

schlafly · 2023-04-26T17:39:58Z

Great! Just out of curiosity---for your simulated data, do we expect to be able to achieve perfect agreement? I feel like I can see a bit of remaining structure by eye in the mosaic, but it looks a bit more like a gradient than an offset, and I wonder if it's an issue in the simulations rather than an issue in the optimization. e.g., something like the simulations having constant sky offsets in terms of sky / pixel, but the code assuming that the sky is constant in terms of sky / unit area, so the pixel scale factor comes into one bit but not the other? Just guessing!

ddavis-stsci

When I try and run the pytests from your branch I get these errors,
TypeError: Input must be a list of either strings (full path to ASDF files) or Roman datamodels.
and I think the pytests should be self contained.

ddavis-stsci · 2023-04-26T15:48:15Z

docs/roman/skymatch/description.rst

+   as well as instrumental (e.g. thermal) background, etc.
+
+The step records information in three keywords that are included in the output
+files:


For asdf files we usually refer to keywords (FITS) as attributes.

Got it, thanks!

ddavis-stsci · 2023-04-26T15:49:36Z

docs/roman/skymatch/description.rst

+
+- BKGLEVEL: the sky level computed for each image
+
+- BKGSUB: a boolean indicating whether or not the sky was subtracted from the


Note: we can expand the attribute names to be longer than the 8 character FITS limit,
so I'd suggest something like
BKGMETH --> background_method

Good point! These aren't the names I gave the attributes, I just hadn't changed this part of the docs to match. Correcting them now.

ddavis-stsci · 2023-04-26T18:11:48Z

romancal/skymatch/skymatch_step.py

+        self.log.setLevel(logging.DEBUG)
+        # for now turn off memory optimization until we have better machinery
+        # to handle outputs in a consistent way.
+        self._is_asn = False


Where is the memory optimization?

This was recently implemented in jwst by @mcara, but not implemented here. I'll remove this comment.

ddavis-stsci · 2023-04-26T18:15:08Z

romancal/skymatch/skymatch_step.py

+        input_image_model = image_model
+
+        if "background" not in image_model.meta:
+            # TODO: remove this block when ``rad`` has a background schema.


Do we have an issue for this in the rad project?

See here. 👍🏻

ddavis-stsci

You also need to add a link to your successful run of the regression tests,
see the "GitHub PR" section of
https://innerspace.stsci.edu/pages/viewpage.action?spaceKey=SCSB&title=Roman+Calibration+Pipeline

bmorris3 · 2023-05-04T15:05:38Z

RE @schlafly in #687 (comment): we discussed this a bit in the last pipeline tag-up, but I'll share my response here so others can see too. The slight mismatches that remain in the demo plot above are likely slightly exaggerated by the visualization I made.

I've updated the visualization, and the notebook that generates the plot below is available here:

Each image is labeled with a random index, and the color is chosen from blue to red for images from low to high mean background flux. Left column is before sky match, right column is after. Histograms in the bottom row show all pixel values in each image. The image that seems like the worst match is image "6", which has a post-subtraction mean normalized by its stddev of 2%.

mcara · 2023-05-05T15:30:23Z

What was matched: mode? mean? median? This can affect the quality of the match that you observe. For 'mode', binwidth is important too.

bmorris3 · 2023-05-09T15:29:32Z

Thanks for the pointers @mcara. I ran the sky match step with:

result = SkyMatchStep.call(image_models, subtract=False, skymethod='global+match')

which would choose the default sky statistic mode. The bin width is also the default 0.1.

mcara · 2023-05-09T16:02:44Z

I would suggest that you re-do the test using skystat='mean' since that is what you use in your script when making those plots.

mcara

It looks like 99.9% of the code from JWST pipeline + PEP8 and style improvements + support for units. Should function like JWST code.

bmorris3 · 2023-05-09T17:09:42Z

I would suggest that you re-do the test using skystat='mean' since that is what you use in your script when making those plots.

Good call @mcara, see below for a version with skystat='mean', which shows even better agreement in this toy example:

schlafly · 2023-05-09T17:11:33Z

Ah, yes, that looks much better!

bmorris3 · 2023-05-10T18:23:29Z

Here's the regression test link.

ddavis-stsci

LGTM

PaulHuwe

I assume regression tests will be done in another PR.

docs/roman/skymatch/description.rst

PaulHuwe · 2023-05-17T16:20:33Z

romancal/skymatch/region.py

+        # temporary) workaround to a limitation of the original code that the
+        # polygon must be completely contained in the image. It seems that the
+        # code works fine if we make sure that the bottom-left corner of the
+        # polygon's bounding box has non-negative coordinates.


Does this mean the code breaks if a polygon with more than 4 sides is used?

PaulHuwe · 2023-05-17T16:23:38Z

romancal/skymatch/region.py

+                self._shiftx = x
+            if y < self._shifty:
+                self._shifty = y
+        v = [(i - self._shiftx, j - self._shifty) for i, j in vertices]


Ah, no the could would shift fine - maybe the above text should be changed a bit? (Since if one has a 6 sided polygon, it isn't moving the bottom-left corner to (0,0), but further positive in order to get other corners in the positive planar quadrant.

romancal/skymatch/region.py

romancal/skymatch/skyimage.py

PaulHuwe · 2023-05-18T00:25:55Z

romancal/skymatch/skymatch_step.py

+                dm.data[...] = sky_image.image[...]
+
+        dm.meta.cal_step.skymatch = step_status
+


Can you make an issue to add skymatch to cal_step in RAD?

PaulHuwe · 2023-05-18T00:27:12Z

romancal/skymatch/skymatch_step.py

+
+        if self._is_asn:
+            dm.save(image)
+            dm.close()


Are you saving the infrastructure to add this to the pipeline for another ticket? (Possibly for a new pipeline - ELPP vs HLPP.)

romancal/skymatch/tests/test_skymatch.py

mcara · 2023-05-18T04:21:49Z

I assume regression tests will be done in another PR.

Regression tests for this step were not available in the JWST pipeline until recently: spacetelescope/jwst#7531 They could be added to this PR or in a separate PR but ideally it should be done in this PR.

Co-authored-by: Paul Huwe <[email protected]>

bmorris3 · 2023-05-19T17:12:33Z

For ease of finding them later, I'll collect upstream issues/links here:

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

nden · 2023-06-05T19:49:09Z

Can this be merged or is anyone interested in reviewing it?

ddavis-stsci · 2023-06-05T19:58:44Z

Does this have a Roman-Developers-Pull-Requests run?
Other than that it has been reviewed several times (I think).

nden · 2023-06-06T17:25:15Z

https://plwishmaster.stsci.edu:8081/job/RT/job/Roman-Developers-Pull-Requests/220/
regression tests are passing

Co-authored-by: Paul Huwe <[email protected]> Co-authored-by: Nadia Dencheva <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

bmorris3 requested a review from a team as a code owner April 26, 2023 13:52

github-actions bot added dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation testing labels Apr 26, 2023

bmorris3 force-pushed the skymatch branch from 5037a86 to 28411d2 Compare April 26, 2023 13:57

braingram mentioned this pull request Apr 26, 2023

check style CI job not using posargs #688

Closed

bmorris3 force-pushed the skymatch branch from 63a6f2b to 3a6437e Compare April 26, 2023 14:12

ddavis-stsci requested changes Apr 26, 2023

View reviewed changes

ddavis-stsci added this to the 23Q3_B10 milestone Apr 26, 2023

ddavis-stsci added the skymatch label Apr 26, 2023

ddavis-stsci assigned bmorris3 Apr 26, 2023

bmorris3 force-pushed the skymatch branch from 3a6437e to 0a428b1 Compare April 28, 2023 18:50

ddavis-stsci requested changes May 4, 2023

View reviewed changes

bmorris3 mentioned this pull request May 4, 2023

Add schema for background spacetelescope/rad#247

Closed

bmorris3 force-pushed the skymatch branch from c523eae to 004e2cd Compare May 4, 2023 15:24

bmorris3 requested review from schlafly, ddavis-stsci and PaulHuwe May 9, 2023 15:16

mcara approved these changes May 9, 2023

View reviewed changes

bmorris3 force-pushed the skymatch branch from 004e2cd to b6a7a85 Compare May 9, 2023 19:00

bmorris3 force-pushed the skymatch branch from b6a7a85 to cf9091c Compare May 11, 2023 14:44

bmorris3 added 5 commits May 16, 2023 08:59

adding changelog entry

413d3a7

style corrections

cfd1909

better comments/docs thanks to Dave Davis

e4531b3

adding skymatch to known suffixes

a3ede08

updating minversion of spherical-geometry to match jwst

b71413f

bmorris3 force-pushed the skymatch branch from 42016f6 to 05df8f3 Compare May 16, 2023 13:00

updating minversion of stsci.imagestats

11018d8

bmorris3 force-pushed the skymatch branch from e371702 to 11018d8 Compare May 16, 2023 13:56

ddavis-stsci approved these changes May 16, 2023

View reviewed changes

PaulHuwe reviewed May 18, 2023

View reviewed changes

bmorris3 and others added 4 commits May 18, 2023 10:20

Apply suggestions from code review

47247ec

Co-authored-by: Paul Huwe <[email protected]>

Update romancal/skymatch/region.py

c136e7b

Co-authored-by: Paul Huwe <[email protected]>

Update romancal/skymatch/region.py

41d7b4a

Co-authored-by: Paul Huwe <[email protected]>

Update romancal/skymatch/region.py

149a5fd

Co-authored-by: Paul Huwe <[email protected]>

bmorris3 mentioned this pull request May 19, 2023

Add cal_step for sky match spacetelescope/rad#265

Closed

PaulHuwe approved these changes May 22, 2023

View reviewed changes

nden and others added 2 commits June 5, 2023 15:48

Merge branch 'main' into skymatch

1ad58d6

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

2efec43

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

ddavis-stsci modified the milestones: 23Q3_B10, 23Q4_B11 Jun 6, 2023

Merge branch 'main' into skymatch

2684287

nden merged commit a026c37 into spacetelescope:main Jun 6, 2023

WilliamJamieson mentioned this pull request Jun 6, 2023

Move skymatch changelog entry to correct version #713

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial sky matching implementation #687

Initial sky matching implementation #687

bmorris3 commented Apr 26, 2023 •

edited

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading

schlafly commented Apr 26, 2023

ddavis-stsci left a comment

ddavis-stsci Apr 26, 2023

bmorris3 May 4, 2023

ddavis-stsci Apr 26, 2023

bmorris3 May 4, 2023

ddavis-stsci Apr 26, 2023

bmorris3 May 4, 2023

ddavis-stsci Apr 26, 2023

bmorris3 May 4, 2023

ddavis-stsci left a comment

bmorris3 commented May 4, 2023

mcara commented May 5, 2023

bmorris3 commented May 9, 2023 •

edited

Loading

mcara commented May 9, 2023

mcara left a comment

bmorris3 commented May 9, 2023

schlafly commented May 9, 2023

bmorris3 commented May 10, 2023

ddavis-stsci left a comment

PaulHuwe left a comment

PaulHuwe May 17, 2023

mcara May 18, 2023

PaulHuwe May 17, 2023

PaulHuwe May 18, 2023

PaulHuwe May 18, 2023

mcara commented May 18, 2023

bmorris3 commented May 19, 2023

nden commented Jun 5, 2023

ddavis-stsci commented Jun 5, 2023

nden commented Jun 6, 2023


		- BKGLEVEL: the sky level computed for each image

		- BKGSUB: a boolean indicating whether or not the sky was subtracted from the

		dm.data[...] = sky_image.image[...]

		dm.meta.cal_step.skymatch = step_status

Initial sky matching implementation #687

Initial sky matching implementation #687

Conversation

bmorris3 commented Apr 26, 2023 • edited Loading

codecov bot commented Apr 26, 2023 • edited Loading

Codecov Report

schlafly commented Apr 26, 2023

ddavis-stsci 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

ddavis-stsci left a comment

Choose a reason for hiding this comment

bmorris3 commented May 4, 2023

mcara commented May 5, 2023

bmorris3 commented May 9, 2023 • edited Loading

mcara commented May 9, 2023

mcara left a comment

Choose a reason for hiding this comment

bmorris3 commented May 9, 2023

schlafly commented May 9, 2023

bmorris3 commented May 10, 2023

ddavis-stsci left a comment

Choose a reason for hiding this comment

PaulHuwe 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

mcara commented May 18, 2023

bmorris3 commented May 19, 2023

nden commented Jun 5, 2023

ddavis-stsci commented Jun 5, 2023

nden commented Jun 6, 2023

bmorris3 commented Apr 26, 2023 •

edited

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading

bmorris3 commented May 9, 2023 •

edited

Loading