RAD-161: Bit Mask to Resample #401

PaulHuwe · 2024-04-01T21:10:43Z

Resolves RAD-161

Closes #399

This PR adds a bit mask field to resample.

Checklist

Schema changes discussed at RAD Review Board meeting
Added entry in CHANGES.rst under the corresponding subsection
Updated relevant roman_datamodels utilities and tests
Passed romancal regression testing on Jenkins / PLWishMaster. Link: https://plwishmaster.stsci.edu:8081/job/RT/job/romancal/XXX/

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

codecov · 2024-04-01T21:13:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.38%. Comparing base (0fb1239) to head (4911a2f).
Report is 14 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #401   +/-   ##
=======================================
  Coverage   95.38%   95.38%           
=======================================
  Files           4        4           
  Lines         195      195           
=======================================
  Hits          186      186           
  Misses          9        9

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

ddavis-stsci

I'm curious why is this only in resample? The first input for the good_bits string is in outlier_detection_step. Shouldn't this be in both places? Are the resample good_bits independent of those in outlier detection?

PaulHuwe · 2024-04-02T13:34:55Z

I'm curious why is this only in resample? The first input for the good_bits string is in outlier_detection_step. Shouldn't this be in both places? Are the resample good_bits independent of those in outlier detection?

@schlafly @mairanteodoro Can you reply?

mairanteodoro · 2024-04-02T15:13:55Z

I'm curious why is this only in resample? The first input for the good_bits string is in outlier_detection_step. Shouldn't this be in both places? Are the resample good_bits independent of those in outlier detection?

I think @ddavis-stsci is correct; the good_bits parameter has to be present in both steps, since resample is called from outlier detection step too. Maybe we should have an additional attribute for the resample parameters used with that specific step?

PaulHuwe · 2024-04-02T18:44:49Z

I'm curious why is this only in resample? The first input for the good_bits string is in outlier_detection_step. Shouldn't this be in both places? Are the resample good_bits independent of those in outlier detection?

I think @ddavis-stsci is correct; the good_bits parameter has to be present in both steps, since resample is called from outlier detection step too. Maybe we should have an additional attribute for the resample parameters used with that specific step?

What are you proposing for the additional resample keywords?

mairanteodoro · 2024-04-02T19:25:02Z

I'm curious why is this only in resample? The first input for the good_bits string is in outlier_detection_step. Shouldn't this be in both places? Are the resample good_bits independent of those in outlier detection?

I think @ddavis-stsci is correct; the good_bits parameter has to be present in both steps, since resample is called from outlier detection step too. Maybe we should have an additional attribute for the resample parameters used with that specific step?

What are you proposing for the additional resample keywords?

Perhaps meta.outlier_detection.resample?

schlafly · 2024-04-03T00:49:56Z

Thanks Dave, Paul, Mairan. Yes, I agree with Dave & Mairan that we need this. It's a little frustrating because I don't think there are any situations where we would want to allow these keywords to be different, but we do have them as separate steps so we should track them separately. The resample bit is only in the L3 products but the outlier rejection occurs on L2 images. The outlier rejection bit should definitely be optional since we don't intend any of the products we actually archive to have that keyword. The individual image metadata in an L3 file would include it, though, in principle; I don't know if that argues for something about how it is included in the metadata.

I think I'd argue for something like meta.outlier_detection.good_bits in the WFI image schema. If we keep this optional, as I think we should, I think we could go ahead and merge this one and make an issue for adding something parallel for outlier detection, since that wouldn't be a schema-breaking change?

PaulHuwe · 2024-04-03T15:38:59Z

Thanks Dave, Paul, Mairan. Yes, I agree with Dave & Mairan that we need this. It's a little frustrating because I don't think there are any situations where we would want to allow these keywords to be different, but we do have them as separate steps so we should track them separately. The resample bit is only in the L3 products but the outlier rejection occurs on L2 images. The outlier rejection bit should definitely be optional since we don't intend any of the products we actually archive to have that keyword. The individual image metadata in an L3 file would include it, though, in principle; I don't know if that argues for something about how it is included in the metadata.

I think I'd argue for something like meta.outlier_detection.good_bits in the WFI image schema. If we keep this optional, as I think we should, I think we could go ahead and merge this one and make an issue for adding something parallel for outlier detection, since that wouldn't be a schema-breaking change?

@mairanteodoro and I chatted about this yesterday, and he requested we simply import the whole resample group within outlier detection (as a "sub-folder" named resample), which would be optional. Accordingly, good_bits would come along for the ride. Do you and @ddavis-stsci agree with this approach?

mairanteodoro · 2024-04-03T15:41:22Z

@mairanteodoro and I chatted about this yesterday, and he requested we simply import the whole resample group within outlier detection (as a "sub-folder" named resample), which would be optional. Accordingly, good_bits would come along for the ride. Do you and @ddavis-stsci agree with this approach?

@PaulHuwe I think we can go with @schlafly's suggestion and have only meta.outlier_detection.good_bits instead.

ddavis-stsci · 2024-04-03T15:52:56Z

I think Eddie's suggestion might be a better approach. Resample and outlier detection may have different bits set by the user for specific science goals and it might be good to have both.

…

On 4/3/24 11:41 AM, mairan wrote: @mairanteodoro <https://urldefense.com/v3/__https://github.com/mairanteodoro__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!w12WEOFm798LQcU0ScWD5RoTB3qyoxypkBmxWz_-xiOJDmQJevuvxfdd6AKq_lz2Ugbp_vXCzuaSnv3ZXA2DRuPy$> and I chatted about this yesterday, and he requested we simply import the whole resample group within outlier detection (as a "sub-folder" named resample), which would be optional. Accordingly, good_bits would come along for the ride. Do you and @ddavis-stsci <https://urldefense.com/v3/__https://github.com/ddavis-stsci__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!w12WEOFm798LQcU0ScWD5RoTB3qyoxypkBmxWz_-xiOJDmQJevuvxfdd6AKq_lz2Ugbp_vXCzuaSnv3ZXFxxRyku$> agree with this approach? @PaulHuwe <https://urldefense.com/v3/__https://github.com/PaulHuwe__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!w12WEOFm798LQcU0ScWD5RoTB3qyoxypkBmxWz_-xiOJDmQJevuvxfdd6AKq_lz2Ugbp_vXCzuaSnv3ZXH7zjR_1$> I think we can go with @schlafly <https://urldefense.com/v3/__https://github.com/schlafly__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!w12WEOFm798LQcU0ScWD5RoTB3qyoxypkBmxWz_-xiOJDmQJevuvxfdd6AKq_lz2Ugbp_vXCzuaSnv3ZXG0-aIql$>'s suggestion instead and have |meta.outlier_detection.good_bits| instead. — Reply to this email directly, view it on GitHub <https://urldefense.com/v3/__https://github.com/spacetelescope/rad/pull/401*issuecomment-2034960414__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!w12WEOFm798LQcU0ScWD5RoTB3qyoxypkBmxWz_-xiOJDmQJevuvxfdd6AKq_lz2Ugbp_vXCzuaSnv3ZXDkW7nLS$>, or unsubscribe <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ALXCXWPFK75EAIPCZC3V7D3Y3QPLTAVCNFSM6AAAAABFSGWBPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZUHE3DANBRGQ__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!w12WEOFm798LQcU0ScWD5RoTB3qyoxypkBmxWz_-xiOJDmQJevuvxfdd6AKq_lz2Ugbp_vXCzuaSnv3ZXCm7_O32$>. You are receiving this because you were mentioned.Message ID: ***@***.***>

PaulHuwe · 2024-04-03T17:39:02Z

@schlafly @mairanteodoro @ddavis-stsci Another wrinkle: there is no outlier_detection keyword group in RAD. Do we want to make one, or do we want to put its good_bits elsewhere?

schlafly · 2024-04-03T19:36:50Z

Yes, I'm suggesting adding it parallel to source detection.

…

On Wed, Apr 3, 2024, 13:39 Paul Huwe ***@***.***> wrote: @schlafly <https://github.com/schlafly> @mairanteodoro <https://github.com/mairanteodoro> @ddavis-stsci <https://github.com/ddavis-stsci> Another wrinkle: there is no outlier_detection keyword group in RAD. Do we want to make one, or do we want to put its good_bits elsewhere? — Reply to this email directly, view it on GitHub <#401 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAK2E4J4XOR66TREWEOQYCLY3Q5EZAVCNFSM6AAAAABFSGWBPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVGIYDEMBQGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

PaulHuwe · 2024-04-03T20:19:13Z

outlider_detection with good_bits added.

mairanteodoro

Looks good, @PaulHuwe! Thanks.
My only suggestion is to mention the new outlier detection group in CHANGES.rst.

CHANGES.rst

schlafly · 2024-04-04T12:54:12Z

src/rad/resources/schemas/outlier_detection-1.0.0.yaml

+    type: string
+propertyOrder: [good_bits]
+flowStyle: block
+required: [good_bits]


Just making sure I understand---good_bits is required if outiler_detection is specified, but outlier_detection is not required in wfi_image. I think that's what you've written and that sounds right to me, but just checking!

Correct, that is what I intended with my implementation.

ddavis-stsci

LGTM

PaulHuwe and others added 3 commits April 1, 2024 17:08

Initial commit.

daa79ad

Updated Changelog.

ea9fed0

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

dd09ca6

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

PaulHuwe marked this pull request as ready for review April 1, 2024 21:17

PaulHuwe requested review from kdupriestsci, jbrookens and a team as code owners April 1, 2024 21:17

stscijgbot-rstdms mentioned this pull request Apr 1, 2024

Add Bit Mask to Resample #399

Closed

ddavis-stsci reviewed Apr 2, 2024

View reviewed changes

kdupriestsci approved these changes Apr 2, 2024

View reviewed changes

PaulHuwe added 3 commits April 3, 2024 16:06

Added outlier detection group and good_bits.

8153b42

Rebased.

5e765a0

Rebased.

88ddcba

PaulHuwe requested review from schlafly, mairanteodoro, ddavis-stsci and kdupriestsci April 3, 2024 20:18

mairanteodoro reviewed Apr 3, 2024

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

Update Changelog.

1925db6

jbrookens approved these changes Apr 3, 2024

View reviewed changes

schlafly reviewed Apr 4, 2024

View reviewed changes

kdupriestsci approved these changes Apr 4, 2024

View reviewed changes

Removed outlier_detection from wfi_mosaic.

4911a2f

ddavis-stsci approved these changes Apr 4, 2024

View reviewed changes

PaulHuwe merged commit 982fe1b into spacetelescope:main Apr 4, 2024
12 checks passed

nden added this to the Build 14 milestone May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RAD-161: Bit Mask to Resample #401

RAD-161: Bit Mask to Resample #401

PaulHuwe commented Apr 1, 2024 •

edited

Loading

codecov bot commented Apr 1, 2024 •

edited

Loading

ddavis-stsci left a comment

PaulHuwe commented Apr 2, 2024

mairanteodoro commented Apr 2, 2024 •

edited

Loading

PaulHuwe commented Apr 2, 2024

mairanteodoro commented Apr 2, 2024

schlafly commented Apr 3, 2024

PaulHuwe commented Apr 3, 2024

mairanteodoro commented Apr 3, 2024 •

edited

Loading

ddavis-stsci commented Apr 3, 2024 via email

PaulHuwe commented Apr 3, 2024

schlafly commented Apr 3, 2024 via email

PaulHuwe commented Apr 3, 2024

mairanteodoro left a comment

schlafly Apr 4, 2024

PaulHuwe Apr 4, 2024

ddavis-stsci left a comment

RAD-161: Bit Mask to Resample #401

RAD-161: Bit Mask to Resample #401

Conversation

PaulHuwe commented Apr 1, 2024 • edited Loading

codecov bot commented Apr 1, 2024 • edited Loading

Codecov Report

ddavis-stsci left a comment

Choose a reason for hiding this comment

PaulHuwe commented Apr 2, 2024

mairanteodoro commented Apr 2, 2024 • edited Loading

PaulHuwe commented Apr 2, 2024

mairanteodoro commented Apr 2, 2024

schlafly commented Apr 3, 2024

PaulHuwe commented Apr 3, 2024

mairanteodoro commented Apr 3, 2024 • edited Loading

ddavis-stsci commented Apr 3, 2024 via email

PaulHuwe commented Apr 3, 2024

schlafly commented Apr 3, 2024 via email

PaulHuwe commented Apr 3, 2024

mairanteodoro left a comment

Choose a reason for hiding this comment

schlafly Apr 4, 2024

Choose a reason for hiding this comment

PaulHuwe Apr 4, 2024

Choose a reason for hiding this comment

ddavis-stsci left a comment

Choose a reason for hiding this comment

PaulHuwe commented Apr 1, 2024 •

edited

Loading

codecov bot commented Apr 1, 2024 •

edited

Loading

mairanteodoro commented Apr 2, 2024 •

edited

Loading

mairanteodoro commented Apr 3, 2024 •

edited

Loading