IGNITE-13386 [ML]: Add new distances (BrayCurtis, Canberra, JensenShannon and etc)#8197
Conversation
zaleslaw
left a comment
zaleslaw
left a comment
There was a problem hiding this comment.
Functionality is good, need to add more comments to avoid problems with merging and TC
| private final Double base; | ||
|
|
||
| public JensenShannonDistance() { | ||
| base = null; |
There was a problem hiding this comment.
Could we add here the default value which could make sense?
There was a problem hiding this comment.
Add Math.E as default value because Math.log(Math.E) == 1 and js /= Math.log(Math.E) is equal to js /= 1
| import org.apache.ignite.ml.math.util.MatrixUtil; | ||
|
|
||
| /** | ||
| * Calculates the JensenShannonDistance distance between two points. |
There was a problem hiding this comment.
Could you please add a link to the Wiki or paper or put the formula in pseudocode (will be useful for understanding)?
There was a problem hiding this comment.
Added link to wikipedia.
| import org.apache.ignite.ml.math.util.MatrixUtil; | ||
|
|
||
| /** | ||
| * Calculates the Canberra distance between two points. |
There was a problem hiding this comment.
Could you please add a link to the Wiki or paper or put the formula in pseudocode (will be useful for understanding)?
There was a problem hiding this comment.
Added link to wikipedia.
| import org.apache.ignite.ml.math.util.MatrixUtil; | ||
|
|
||
| /** | ||
| * Calculates the Bray Curtis distance between two points. |
There was a problem hiding this comment.
Could you please add a link to the Wiki or paper or put the formula in pseudocode (will be useful for understanding)?
There was a problem hiding this comment.
Added link to wikipedia.
| import static org.junit.Assert.assertEquals; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class BrayCurtisDistanceTest { |
There was a problem hiding this comment.
Add comment like
/**
* Evaluate BrayCurtisDistance in multiple test datasets
*/
|
|
||
| private final TestData testData; | ||
|
|
||
| public CanberraDistanceTest(TestData testData) { |
There was a problem hiding this comment.
Add comment like
/** */
| new BrayCurtisDistance(), | ||
| new CanberraDistance(), | ||
| new JensenShannonDistance(), | ||
| new WeightedMinkowskiDistance(4, new DenseVector(new double[]{1, 1, 1})), |
There was a problem hiding this comment.
Maybe these parameters could become default?
There was a problem hiding this comment.
We can set default value for p (power) parameter as 2, but weight can not be default because weight's is dimensional dependent. If we want to evaluate distance between d(a,b), then a.size() == b.size() && a.size() == weight.size().
Or I can use null for weight and use them only when weight != null.
| import static org.junit.Assert.assertEquals; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class JensenShannonDistanceTest { |
There was a problem hiding this comment.
Add comment like
/** */
| import static org.junit.Assert.assertEquals; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class WeightedMinkowskiDistanceTest { |
There was a problem hiding this comment.
Add comment like
/** */
| } | ||
|
|
||
| @Test | ||
| public void test() { |
There was a problem hiding this comment.
change the test name to test+, because it could create problems with next migration to next versions of JUNIt framework
There was a problem hiding this comment.
I renamed my tests like test<distanceName.
mrk-andreev
force-pushed
the
IGNITE-13386
branch
3 times, most recently
from
fde8a4b to
7c86935
Compare
zaleslaw
changed the title
zaleslaw
changed the title
mrk-andreev
force-pushed
the
IGNITE-13386
branch
from
7c86935 to
b7d1583
Compare
2 participants
add distances
Issue: https://issues.apache.org/jira/browse/IGNITE-13386