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

Address result type mismatch #104

Closed
dchaley opened this issue Jan 3, 2024 · 0 comments · Fixed by #117
Closed

Address result type mismatch #104

dchaley opened this issue Jan 3, 2024 · 0 comments · Fixed by #117

Comments

@dchaley
Copy link
Owner

dchaley commented Jan 3, 2024

The test test_two_image_peaks asserts that out.dtype == _supported_float_type(mask.dtype). Meanwhile the current reconstruct implementation indeed creates the result image as a float, even if it was ints to begin with.

I'm not sure we need to (always?) do this. The core of the algorithm is to adjust to the neighborhood max. The max can't have more precision than any of the starting numbers. And the max can't be capped to more precision than the mask precision. So if ints are masking ints, why not have int results?

However floats masked by ints are floats, and arguably ints masked by floats should be floats. for example 10 mask 0.51 should be 5.1 not 5. (Right?)

The current behavior is to always return floats. This is undesirable for performance as it precludes updating in-place. I wonder if we can simply ship this as new behavior. Downstream usages could be affected if they assume floats and start getting ints. Can we control via "Yet Another Parameter"™️ ?

@dchaley dchaley changed the title Investigate algorithm failures Investigate reconstruct algorithm failures Jan 3, 2024
@dchaley dchaley changed the title Investigate reconstruct algorithm failures Address result type mismatch Jan 15, 2024
dchaley added a commit that referenced this issue Jan 16, 2024
The current implementation outputs results as floats even if the input
was ints. We don't want that for our optimizations, see also:
#104

but we do need to maintain it in the existing API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

1 participant