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

Raise Error when MDFmetric is used in QB or QBX #1440

Merged
merged 2 commits into from Mar 4, 2018

Conversation

Projects
None yet
3 participants
@Garyfallidis
Member

Garyfallidis commented Mar 3, 2018

…Actually the AveragePointwiseEuclideanMetric has the correct behavior of the MDF metric as explained in the QuickBundles paper. @MarcCote this one is for you to check.

@Garyfallidis Garyfallidis requested a review from MarcCote Mar 3, 2018

@MarcCote

I don't think you should remove the MDF metric as it is the only way right now to compute the distance between two streamlines unless you manually do the direct-flip comparison yourself (I'm talking without using Quickbundles).

To reduce ambiguity when using QB, we could raise an exception when isinstance(metric, MDF) and tell the user to use AveragePointwiseEuclideanMetric.

And to really fix the ambiguity we need to separate the metric used to compare streamlines (with centroids) and the metric used when merging a streamline to a new cluster (updating the centroid).

@@ -69,6 +73,8 @@ def get_streamlines():
print("Cluster sizes:", map(len, clusters))
"""
If there are the data do not need ordering checks then make sure that

This comment has been minimized.

@MarcCote

MarcCote Mar 3, 2018

Contributor

wording

@@ -47,7 +47,11 @@ def get_streamlines():
`Feature` is the `IdentityFeature` the streamlines won't be resampled thus
saving some computational time.
**Note:** Inputs must be sequences of same length.
**Note:** Inputs must be sequences of same length. Also when the input
feature of this metric ``is_order_invariant=True`` then the metric is

This comment has been minimized.

@MarcCote

MarcCote Mar 3, 2018

Contributor

That's misleading. It's not because the Features object has is_order_invariant=True that this metric becomes the MDF. However, it is true it would produce the same result.

That said, I don't see how you can design a Feature object that would allow you to produce the same results as the MDF described in [Garyfallidis12]_.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 4, 2018

The main problem is that if someone reads the paper may go and use the MDF metric as input to QuickBundles although one should be using the AverageEuclidian metric. So, I think we should at least remove the MDF metric from the same tutorial with AverageEuclidian.

And I am not sure with using is isinstance(metric, MDF) but I can try it and see how it goes. Let me update this PR.... one sec...

@Garyfallidis Garyfallidis force-pushed the Garyfallidis:fix_MDF_ambiguity branch from 1f56c6d to 5d01d96 Mar 4, 2018

@Garyfallidis Garyfallidis changed the title from Removing MDF metric as it can be ambiguous in the current framework. … to Raise Error when MDFmetric is used in QB or QBX Mar 4, 2018

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 4, 2018

What about now @MarcCote ?

@MarcCote

@Garyfallidis yes this is better IMO.

::
MDF distance between the first two streamlines: 11.681308709622542
.. _clustering-examples-MinimumAverageDirectFlipMetric:

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2018

Contributor

This line should be removed too, no?

@@ -119,42 +119,8 @@ def get_streamlines():
.. _clustering-examples-MinimumAverageDirectFlipMetric:

This comment has been minimized.

@MarcCote

MarcCote Mar 4, 2018

Contributor

You have the same line below.

@codecov-io

This comment has been minimized.

codecov-io commented Mar 4, 2018

Codecov Report

Merging #1440 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1440      +/-   ##
==========================================
+ Coverage   87.41%   87.41%   +<.01%     
==========================================
  Files         239      239              
  Lines       30568    30579      +11     
  Branches     3289     3291       +2     
==========================================
+ Hits        26720    26731      +11     
  Misses       3078     3078              
  Partials      770      770
Impacted Files Coverage Δ
dipy/segment/tests/test_qbx.py 96.92% <100%> (+0.14%) ⬆️
dipy/segment/clustering.py 93.66% <100%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9fa0bd...aa1db45. Read the comment docs.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 4, 2018

Thanks! Done with removing the two lines.

@MarcCote MarcCote merged commit 6a78d50 into nipy:master Mar 4, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.41%)
Details
codecov/project 87.41% (+<.01%) compared to f9fa0bd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Merge pull request nipy#1440 from Garyfallidis/fix_MDF_ambiguity
Raise Error when MDFmetric is used in QB or QBX
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment