Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-45144: Fix support for weight arrays in AccumulatorMeanStack.add_masked_image() #381

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Jul 8, 2024

No description provided.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor improvement suggestions, and I'd like to request one unit test involving a non-trivial array as weight. I'm thinking of a toy case where the image and variance are identical (but have pixel-to-pixel variance) and passing the image as the weight produces the same stacked image as the stacked variance plane where the weight is 1.0

But more generally, what is the use case to support weight that is an array? Neither in adding per-detector warps to make a warp, nor in coadding warps do we want to use a weight that varies from one pixel to another.

@@ -106,25 +106,37 @@ def add_masked_image(self, masked_image, weight=1.0):
good_pixels = np.where(((masked_image.mask.array & self.bit_mask_value) == 0)
& np.isfinite(masked_image.mask.array))

self.sum_weight[good_pixels] += weight
self.sum_wdata[good_pixels] += weight*masked_image.image.array[good_pixels]
weight_array = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is 100% internal, but i'd suggest calling it is_weight_array to denote that its meant to be boolean.

afw_masked_image.variance.array)
testing.assert_array_equal(online_masked_image.mask.array,
afw_masked_image.mask.array)
for use_weight_arrays in [False, True]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere, I'd recommend making use_weight_arrays an argument to the test function and use lsst.utils.tests.methodParameters.

@erykoff
Copy link
Contributor Author

erykoff commented Jul 9, 2024

cc @esheldon for the use of the weight array

@arunkannawadi
Copy link
Member

It was the 3-year ago me that made you put this comment Eli. I'm sorry, I don't think that was a good suggestion: #263 (comment)

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring only changes (mostly) look good to me!

@erykoff erykoff merged commit 034a557 into main Jul 19, 2024
2 checks passed
@erykoff erykoff deleted the tickets/DM-45144 branch July 19, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants