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

Avoid overflow in addition of bspline offset for integer-type converters #179

Merged
merged 49 commits into from
Nov 5, 2023

Conversation

joeljonsson
Copy link
Collaborator

Resolves #178

The bspline offset is now computed based on the range of values in the input image, and only if smoothing is enabled (lambda>0).

Small revision of examples:

  • Example_get_apr* now use converters of type float.
  • Added -auto_parameters option to Example_get_apr_by_block
  • Addition of gaussian noise to the piecewise constant reconstruction in Example_reconstruct_image is now controlled via the option -noise <sigma> where the standard deviation can be set (default 0 = no additional noise)

joeljonsson and others added 30 commits October 24, 2022 13:28
to avoid undefined symbol errors in certain uses of the library
to allow access for pickling in pyapr
to avoid overflows when the input image has values near the largest
representable number. New behavior is that no offset is added when
either a) `lambda<=0` or b) the image covers the entire range of values.
required for `FetchContent_MakeAvailable` when building gtest
If not externally available, gtest is downlaoded and built using
CMake's FetchContent
@cheesema
Copy link
Member

cheesema commented Nov 5, 2023

So the current failing test seem to be down to:

https://github.com/cheesema/AdaptiveParticleRepresentation/blob/40e807b87e8d57f1cc4ae14a9c3f6a38286c61ca/src/algorithm/LocalParticleCellSet.hpp#L136

In the sphere2D case; the local intensity scale is exactly 0. This results in nan's, it seem like all other systems this results in 0's for the the later level function. However, in macosx, it seems this now behaves differently.

Tests passing can be recovered by adding 1 to the 0 entries in the LIS.

I am reluctant to add another operation in main, to all paths to handle this case without thought.

@joeljonsson what do you think?

@cheesema
Copy link
Member

cheesema commented Nov 5, 2023

So it seemed the thesholds on this example were set to 0's. Which should never really be the case, but did not seem to matter up to this point.

@joeljonsson
Copy link
Collaborator Author

It seems that all test files are computed with sigma_th=0 and sigma_th_max=0. Clearly having tests rely on undefined behavior giving a certain result is a bad idea. This might be the case in other tests as well, so it is probably best to replace all of the APR test files (and corresponding output_steps images), e.g. with ones computed using autoparameters.

As discussed before we should also add a warning, e.g. here, when both sigma_th and sigma_th_max are 0 that this may lead to undefined behavior.

@cheesema
Copy link
Member

cheesema commented Nov 5, 2023

Added the warning and a test. To be honest; changing all the test files seems more dangerous, let along more work; then this current fix for now.

project(APR DESCRIPTION "Adaptive Particle Representation library")

message(STATUS "CMAKE VERSION ${CMAKE_VERSION}")

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

if(POLICY CMP0135)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this needed to be added?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly I'm not sure, but I think it has to do with the way gtest is now downloaded

@joeljonsson joeljonsson merged commit 2e95a62 into master Nov 5, 2023
3 checks passed
@joeljonsson joeljonsson deleted the patch_178 branch November 9, 2023 14:44
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.

Unsigned Integer Overflow
2 participants