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

Fixes #715 #719

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@MarcCote
Contributor

MarcCote commented Sep 23, 2015

Fixes #715, plus fixes some typos.

"""
qb = QuickBundles(threshold=10.)
feature = ResampleFeature(nb_points=12)
metric = AveragePointwiseEuclideanMetric(feature=feature) # a.k.a. MDF

This comment has been minimized.

@arokem

arokem Sep 24, 2015

Member

Regarding the comment at the end of this line: wouldn't it be better to change the references above to MDF to talk about this metric instead? It's not like 'MDF' is a very commonly used acronym.

@arokem

This comment has been minimized.

Member

arokem commented Sep 24, 2015

Yep - overall a good change to make, I think. With the one comment I made above about 'MDF'. My other comment is that it is now not entirely clear that this is emulating the default behavior, so you might want to point out which lines of code here would happen automatically, and what it would look like if you chose another setting (e.g. nb_points=18). One possibility is to do this with and without the ResampleFeature explicitly mentioned, and show the (small, I assume) differences.

@arokem

This comment has been minimized.

Member

arokem commented Sep 24, 2015

I see that this has some overlap with #682 ;-)

@Garyfallidis - what do you think about these two? In particular, where do you stand on #682?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Sep 24, 2015

Personally I prefer how segment_quickbundles.py tutorial is written in #682. That is to only show the default behaviour and mention in the comment above that the number of points can be changed. Then refer the user to the other tutorial that has more explanation.

Another reason I prefer not showing explicitly ResampleFeature here is that the main goal of this tutorial is to show how simple it is to use the plain QuickBundles. Also, the default behaviour should be alright for the common users.

@Garyfallidis when you say "this makes it difficult for people who were used to the previous version of the algorithm." are you only talking about you :P (not a common user ;)) ? If you want we could make a function quickbundles(streamlines, threshold, nb_points=12) that returns a list of clusters (like the cluster method of a QuickBundles object). What do you think?

@arokem if this one gets in, I'll rebase to update #682 .

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 7, 2015

Hi @MarcCote, can you remove lines
+feature = ResampleFeature(nb_points=12)
+metric = AveragePointwiseEuclideanMetric(feature=feature) # a.k.a. MDF
and I will merge the rest of your corrections.
Make sure this tutorial has a link to the next.

@MarcCote MarcCote force-pushed the MarcCote:fix_qb_docs branch from b82a31e to af39672 Oct 9, 2015

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Oct 9, 2015

This PR is now outdated since #682 is merged.

@MarcCote MarcCote closed this Oct 9, 2015

@MarcCote MarcCote deleted the MarcCote:fix_qb_docs branch Oct 9, 2015

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