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-35135: cpCombine partial reads interferes with amplifier-level scaling #136

Merged
merged 1 commit into from Jun 9, 2022

Conversation

czwa
Copy link
Contributor

@czwa czwa commented Jun 9, 2022

Convert per-amp scales to an image.

This image can then be subset to match the subregion used in the data,
avoiding potential geometry mismatch.

@czwa czwa requested a review from erykoff June 9, 2022 20:13
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

Looks good! I am wondering about performance and if there's any caching that could be done, but it's probably not worth it unless this somehow becomes a pain point.

ampIm /= ampScale
else:
mi /= scale
scaleExp.image[amp.getBBox()] = ampScale
Copy link
Contributor

Choose a reason for hiding this comment

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

So this creates a fake full exposure and fills it with the amp scale values?

And I guess the performance penalty is that it has to do this every time we call applyScale? Will the scale_list be different for every input exposure, or is there some caching that could be done? (And feel free to tell me that this would be premature optimization anyway...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. I thought of trying to be clever, but that seems like I'm just making more places to get geometry wrong.
We do instantiate a new full-size image for each applyScale call, which means we make (Nrows / NsubRows) copies of the same image for each input. This isn't optimal, but pulling it out of the loop will cause the same memory issues the partial reads fixed. The scales are different for each input exposure, so I'm not sure there's any way to optimize there.

This image can then be subset to match the subregion used in the data,
avoiding potential geometry mismatch.
@czwa czwa merged commit 016051e into main Jun 9, 2022
@czwa czwa deleted the tickets/DM-35135 branch June 9, 2022 22:55
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.

None yet

2 participants