FIX Major bugfix in python refinement #377
Conversation
In the pure python refinement code, the image slices were done with floats, which get floored to an int by numpy, instead of the intened round operation.
Fixes warning at multiplication with np.bool array.
|
👍 makes sense to me. @danielballan or @nkeim should merge this as I am only passingly familiar with this code. |
|
In view of the API change this will give, some more testing data below. Apparently, the numba code was made more accurate by #242 without us realizing it. Now the python code is also fixed. I guess this pure python refine could use some streamlining, but that is for another time. this PR: v0.3.0: v0.2.4: |
|
@caspervdw This is great!! Thank you for tracking this down. Your test results suggest that there are reliable upper bounds on positional error. Do you think this would work as an automated test? At this point I think that creating an issue would suffice. |
|
I was thinking the same. There is a test already, |
|
Probably. If you have the code that generated those plots handy, maybe we should start a stash of "human-in-the-loop" tests to be run before releases and on PRs that touch critical code. |
|
Shall we add this to v0.3.1? |
|
Is the signal-to-noise ratio on the plots actually a noise-to-signal ratio? The trends are the opposite of what I would expect, unless I am confused. My vote is that this is a bug fix, not an API change, and thus can go into v0.3.1 (not v0.4.0). |
|
I am also 👍 on this being a bugfix, not an API change. On Sun, Jun 19, 2016 at 5:49 PM Dan Allan [email protected] wrote:
|
|
@danielballan Yes it should be noise-to-signal ratio... I will have a quick look at |
|
Done, ready to merge |
|
Sorry for taking a closer look so late. It looks like this removes the shift code that was the principal difference between the python and numba engines, going all the way back to the beginning. I had always thought that this made the python engine slightly superior! But the data don't lie. |
|
I'll quickly run a test today to check if there is a difference @nkeim |
|
Apparently, it really doesn't make a difference whether you interpolate or not:
Or, the differences with numba (negative = better, positive = worse): |
|
Interesting! I had wondered about that too. It's in the original Crocker-Grier implementation but, IIRC correctly, not in Blair's code. I wonder if Crocker tried it without interpolation first or what motivated that in the first place. |
| # order=2, mode='constant', cval=0) | ||
| # new_coord = coord + off_center | ||
| # # Disallow any whole-pixels moves on future iterations. | ||
| # allow_moves = False |
There was a problem hiding this comment.
All references to allow_moves can be removed. It will always be True.
There was a problem hiding this comment.
I'll look at it and refactor. Also the shift_thresh and break_thresh will be the same thing: if the code does not shift, it will break (as in break on line 371). Calculating the CoM without shifting makes no sense because the outcome will not change.
To test specific numba refine codes.
Drop `break_thresh` because it is not used anymore. Add deprecation warning to `refine`, change API of `_refine` and numba codes.
|
Done the refactor at @danielballan suggestion. Because the interpolation went out, the refinement loop could be simplified substantially: I ran the tests again and the performance is better than before. Maybe I solved another bug: the
|
|
The test should be fixed now, the resulting tracking errors are now lower, agreeing much better (within 10%) with the theoretical ones. |
|
Could anyone check this and merge? The amount of code that is changed is substantial, but also fairly straightforward. I think that this increase of accuracy should be released asap so that people can make use of it. Potentially as a v0.4, not v0.3.1? What do you think? This PR will result in different refined coordinates. |
|
I've looked at the code and am happy with it. If I had more time I would test this with my application (1D feature finding). But if there is a problem with that very rare use case, I will find it soon enough. So I also want to merge if we are OK with this being in v0.3.1. |
|
Yep, this is a bugfix and can go into v0.3.1. Merging! |
Because of the refine refactor in soft-matter#377, one iteratation is the same as no iteration previously. Zero iterations is not possible, because the center of mass is then undefined.
This removes any discrepancy between the python and numba engines:
Fixes #376