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

Bad condition in local tracking #1566

Closed
nilgoyette opened this Issue Jun 14, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@nilgoyette
Copy link
Contributor

nilgoyette commented Jun 14, 2018

In probabilistic_direction_getter.pyx, you have this block:

max_idx = 0
max_value = 0.0
for i in range(_len):
    if bool_array[i] > 0 and pmf[i] > max_value:
        max_idx = i
        max_value = pmf[i]

if pmf[max_idx] == 0:
    return 1
    
good direction!

I think there's a problem with the last condition.

  • In simple terms, the loop is a argmax with a zip(bool_array), so we find the argmax if it's a maximal value AND there's a 1 on the bool_array at the right place.
  • The condition doesn't check if a max_value was found for real. It doesn't care if the inner condition is always False. It simply check if pmf[max_idx] == 0, which can happen even with a zeroed-out bool_array.

We would reach good direction! without problem with this hypothetical case:

pmf        0.4 0.1 0.0 0.0 ... 0.0
bool_array  F   F   ...wathever...

After a discussion with @jchoude, we think that if max_value > 0.0 is a good fix. It would force the argmax to have found an actual maximal value. Can someone confirm that this would be a correct/valid solution? I mean, in the anatomical sense. I'm no tracking expert myself so I prefer asking.

@nilgoyette

This comment has been minimized.

Copy link
Contributor

nilgoyette commented Jun 27, 2018

Anyone on this? My tests indicate that this condition happens not that frequently, but there's a lot of voxels in an image so it adds a lot of wrong stream;ines. If it's a bug, then it's an important bug. @gabknight, @arokem

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Jun 27, 2018

@nilgoyette : where exactly are these lines of code? They're not in 'probabilistic_direction_getter.pyx', as far as I can tell.

@gabknight

This comment has been minimized.

Copy link
Contributor

gabknight commented Jun 27, 2018

Thanks @nilgoyette for pointing this out. I don't think there is bug here. if max_value was never found, it means all value are 0, including pmf[0], leading to return 1. This fails if pmf has negative values; it should not.

This should be clarified in any case. Something like this:

...

if pmf[max_idx] == 0 or max_value == 0:
    return 1

... 
good direction

@arokem here

@nilgoyette

This comment has been minimized.

Copy link
Contributor

nilgoyette commented Jun 27, 2018

@gabknight I always try to doubt myself a little, but I'm quite sure that there's a problem here. I added a condition and a print and I see that it happens. Add the bool_array into your reasoning and check my "hypothetical case".

We do agree that the condition should be clarified, so that's all right with me :)

@jchoude

This comment has been minimized.

Copy link
Contributor

jchoude commented Jun 27, 2018

@gabknight The issue is that if, for example, bool_array[0] == 0, the first value is never assessed. But then, if for some bizarre reason, the rest of the pmf where bool_array > 0 is 0, no new max_index is set. If that's the case, then, at the end, max_idx == 0. If for some reason pmf[0] is still >0, then the following if is avoided, and a direction is picker.

I agree that this is a "rare" case, but I couldn't find any guarantee earlier in the code that pmf[bool_array ==0] == 0. Which, in fact, makes sense, since bool_array could be set for a specific search without changing the pmf.

That yields to streamlines that are still generated even when they shouldn't.

@gabknight

This comment has been minimized.

Copy link
Contributor

gabknight commented Jun 28, 2018

OK, yes. There is a bug here! I will do a PR to fix it.

Thx @nilgoyette @jchoude.

gabknight added a commit to gabknight/dipy that referenced this issue Jun 29, 2018

@arokem arokem closed this in #1578 Jun 29, 2018

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this issue Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment