BUG: make resize with order=0 more robust to floating-point rounding #2648
Conversation
…error As reported in scikit-imagegh-2629, which neighbor is judged to be nearest is currently dependent on whether numpy was built with MKL or OpenBLAS. By shifting coordinates by ~100 times machine epsilon when order=0, this inconsistency can be avoided.
|
Hello @grlee77! Thanks for updating the PR.
Comment last updated on May 05, 2017 at 04:33 Hours UTC |
|
not sure what is causing the Travis failures. they don't seem related to this PR. e.g. |
|
Seems acceptable to me. @mcquin Could you verify this fix? |
|
I don't have access to those computers until next week. I'll verify first
thing on Monday.
…On May 11, 2017 05:12, "Egor Panfilov" ***@***.***> wrote:
Seems acceptable to me.
@mcquin <https://github.com/mcquin> Could you verify this fix?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2648 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEgMW33DHYtvisCzsibRzO2o6zBgDh3Oks5r4tDqgaJpZM4NQJaX>
.
|
|
Have you tested this for larger images as well? Something with several thousand pixels and/or larger scaling factors. |
Not yet, but I will try it. Trying to account for these type of cases are why I had bumped up the offset to a much larger value than the minimum required for the specific example raised in #2629 in my second commit. |
|
Since this is a rather critical fix that seems brittle wrt. the dimensions of the image, I suggest to do further experiments here regarding different image sizes? |
|
Hey @grlee77, with this change I get this result on OS X and Ubuntu 16.04. |
grlee77
changed the title
|
please do not merge yet. Despite the apparent consistency with this toy example, in initial testing with larger sizes and scale factors I do not seem to always get identical results. |
in the toy examplePython 2.7, This patch gives consistent results across two different numpy builds. There are a total of two unique results over two numpy builds and two branches of scikit-image. In a larger examplerandomized labels generated as: sz = (32, 16384)
labels = np.random.rand(*sz)*4
labels = labels.astype(np.int)results of resizing are saved out to master, numpy (defaults) : result 1 So, with this PR, the result becomes consistent across numpy builds, however, unlike for the toy example the result for this branch is not identical to the result from the master branch for either numpy build. I suppose if the current behavior in master is considered a bug due to susceptibility to rounding error then this is not a problem in terms of backwards compatibliity. consistency across numpy builds was also verified for other extreme reshape cases such as (32, 32) reshaped to (32, 100001), etc. |
grlee77
changed the title
|
@ahojnnes Note that the coordinate shifts are only applied when |
|
Hi @grlee77 et al! Although I very much appreciate the effort that's gone into this PR and all the testing in #2629, I most agree with @mcquin's comment here:
The proposed solution, in particular, can be qualified only as a hack, and not something I'm particularly keen to add to skimage. (I don't mean this disrespectfully btw — we all use hacks. But the standard for adding to skimage should be higher.) If we do decide that we need a solution, I would aim for something that looks at the underlying linear algebra problem being solved (is it a matrix inversion?), and what the solutions are to improve that algorithm's numerical stability. |
Agreed. Unfortunately, I don't have the time/motivation to dig deeply into this edge case further at the moment. I understand if others do not wish to merge it as is. |
|
If the decision is not to merge, then maybe it is time to close the PR, isn't it? |
|
@jni I see your point, but at the same time this is a very simple modification which reduces the occurrence of a potentially nasty bug. More importantly: can we imagine any situations in which this modification would break things or propagate errors? |
scikit-image PR 2648 [1] reports a bug that is similar to the one discovered in this project. Compiling scikit-image with that PR seems to solve the discrepancy found between the two computers. Test: $ git clone https://github.com/scikit-image/scikit-image.git $ cd scikit-image $ git fetch-pr origin 2648 $ git checkout origin-pr-2648 $ mkvirtualenv scikit-debug $ pip install -r requirements.txt $ pip install Cython==0.23 $ pip install jupyter $ pip install -e . $ cd {directory containing script-resize.py} $ ./script-resize.py $ scp {image from computerA to computerB} $ jupyter notebook Load CompareImages.ipynb [1] scikit-image/scikit-image#2648
scikit-image PR 2648 [1] reports a bug that is similar to the one discovered in this project. Compiling scikit-image with that PR seems to solve the discrepancy found between the two computers. Test: $ git clone https://github.com/scikit-image/scikit-image.git $ cd scikit-image $ git fetch-pr origin 2648 $ git checkout origin-pr-2648 $ mkvirtualenv scikit-debug $ pip install -r requirements.txt $ pip install Cython==0.23 $ pip install jupyter $ pip install -e . $ cd {directory containing script-resize.py} $ ./script-resize.py $ scp {image from computerA to computerB} $ jupyter notebook Load CompareImages.ipynb [1] scikit-image/scikit-image#2648
rfezzani
added
🩹 type: Bug fix
|
This was not a very robust solution, so I have closed this PR |
|
Thanks, Greg, I'm sorry we could not figure out a cleaner way to address this—tricky! |
Description
As reported in gh-2629, which neighbor is judged to be nearest is currently dependent on
whether numpy was built with MKL or OpenBLAS. By shifting coordinates by ~100 times machine
epsilon when order=0, this inconsistency can be avoided.
The amount of shift was determined empirically as one which gives the same result across MKL and OpenBLAS numpy builds (as tested in parallel conda environments).
I'm not sure there is a good way to properly test this fix across BLAS builds, but I did add a test verifying that for the specific example of gh-2629, the transform gives the same result for both the 2D and n-dimensional code paths.