search

spacetelescope / rad

Nancy Grace Roman Space Telescope shared attributes for processing and archive
https://rad.readthedocs.io/
Other
5 stars 22 forks source link

RAD-161: Bit Mask to Resample #401

Closed PaulHuwe closed 7 months ago

PaulHuwe commented 7 months ago

Resolves RAD-161

Closes #399

This PR adds a bit mask field to resample.

Checklist

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #401 +/- ## ======================================= Coverage 95.38% 95.38% ======================================= Files 4 4 Lines 195 195 ======================================= Hits 186 186 Misses 9 9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

PaulHuwe commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 https://github.com/spacetelescope/rad/pull/401#issuecomment-2035202002, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAK2E4J4XOR66TREWEOQYCLY3Q5EZAVCNFSM6AAAAABFSGWBPWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMZVGIYDEMBQGI . You are receiving this because you were mentioned.Message ID: @.***>

PaulHuwe commented 7 months ago

outlider_detection with good_bits added.