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

minmax_norm in peaks_directions does nothing #389

Closed
samuelstjean opened this issue Jul 8, 2014 · 4 comments · Fixed by #1923

Comments

@samuelstjean
Copy link
Contributor

commented Jul 8, 2014

In the function dipy.recont.peaks.peak_directions, the option minmax_norm does not seem used. A proper normalization is done in peaks from model, but the old, inner function still accepts the arguments without doing anything.

The doc doesn't even speak about it anymore. So should it be removed, or should it be re-enabled so that one can use the function on an odf (while using peaks_from model on an odf doesn't really seem intuitive)?

@jchoude

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

Bump! I encountered exactly the same thing.

@Garyfallidis @arokem What should be the behavior here? I think it should directly be done in this function, to allow users to use precomputed ODFs without going through peaks_from_model (which is my current use case).

@arokem

This comment has been minimized.

Copy link
Member

commented Feb 16, 2016

Just to understand, do you suggest doing something like this:

https://github.com/nipy/dipy/blob/master/dipy/direction/peaks.py#L468

in here:

https://github.com/nipy/dipy/blob/master/dipy/direction/peaks.py#L92

Seems right to me.

@jchoude

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

Well, it's either we do that when minmax_norm=True, or we remove the argument in peak_directions. Right now, there is no effect of setting or not setting the param.

Furthermore, I would personnally change the default to False, but that's debattable. If you want to extract the AFD along the peak directions, you need the values to not be normalized.

@arokem

This comment has been minimized.

Copy link
Member

commented Feb 16, 2016

I am for adding this as an option (default as False seems fine to me --
it's the current behavior anyhow ;D). Do you want to make PR with that?

On Tue, Feb 16, 2016 at 9:01 AM, Jean-Christophe Houde <
notifications@github.com> wrote:

Well, it's either we do that when minmax_norm=True, or we remove the
argument in peak_directions. Right now, there is no effect of setting or
not setting the param.

Furthermore, I would personnally change the default to False, but that's
debattable. If you want to extract the AFD along the peak directions, you
need the values to not be normalized.


Reply to this email directly or view it on GitHub
#389 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.