Raise Error when MDFmetric is used in QB or QBX #1440
Conversation
There was a problem hiding this comment.
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 | |||
| @@ -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 | |||
There was a problem hiding this comment.
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]_.
|
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
force-pushed
the
fix_MDF_ambiguity
branch
from
1f56c6d
to
5d01d96
Compare
Garyfallidis
changed the title
|
What about now @MarcCote ? |
| :: | ||
|
|
||
| MDF distance between the first two streamlines: 11.681308709622542 | ||
|
|
||
| .. _clustering-examples-MinimumAverageDirectFlipMetric: |
There was a problem hiding this comment.
This line should be removed too, no?
| @@ -119,42 +119,8 @@ def get_streamlines(): | |||
|
|
|||
| .. _clustering-examples-MinimumAverageDirectFlipMetric: | |||
There was a problem hiding this comment.
You have the same line below.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
Thanks! Done with removing the two lines. |
Raise Error when MDFmetric is used in QB or QBX
…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.