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-41884: Use more memory efficient nanquantile for large pixel spread. #71

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Nov 28, 2023

No description provided.

Copy link
Contributor

@mfisherlevine mfisherlevine left a comment

Choose a reason for hiding this comment

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

Sorry this took so long to get to. So, I think the code changes look great, thanks. My only question is whether 131072 is the right point to switch over to the other method or not. I tried doing a bit of quick profiling, but couldn't reproduce (or rather, catch) the huge memory usage we were seeing before with the notebook tools I was using. Ideally, we'd have some measure of where the memory usage gets silly, and how the runtimes compare, so that we could make some more informed choices, but I realise that's quite a bit of scope creep to drop on the ticket in the review.

So, I think my question is: given we have 18 bit ADCs, gains close to 1, and don't expect to go wildly negative, 2^18=262144 might be a better choice than the (presumably?) equally arbitrary 2^17 we have now, right? I'm just thinking that we'll routinely see numbers down to ~zero, and above 131072, meaning we'll tend to be in the switchover case most of the time, and we'd probably regain the performance boosts if we upped that, not to mention that we'd then be in a situation where the normal behaviour would be consistent, rather than routinely switching between the two modes based on the image values, and would only switch over in the case of pathological images.

So, assuming this wasn't done for deep reasons (like you having profiled or worked out where the other mode gets sad), I think we should up it buy a factor of two (or actually maybe a little more - presumably there's nothing too magic about powers of two here, so maybe 300k would be better so we can handle ADC_MAX * GAIN). And actually, given that, just making a logger at the point and logging a warning that the image contains unusual values and we're changing the binning mode, would, I think, be really nice too.

@erykoff erykoff merged commit c69f3f1 into main Dec 11, 2023
1 check passed
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