SRV changes #1525
SRV changes #1525
Conversation
- Put a check in SRV Transform method to ensure that there are no consecutive duplicate points in a curve.
- Made change in SRV Transform method to change the dimensionally of encoding ambient space if the Metric used is of instance type SRV Metric.
- Introduced a new method predict_labels in Riemannian Mean Shift class to predict labels for the points passed as an argument.
1. Throw an exception if the curve passed to SRV Transform has consecutive duplicate points 2. Fix the closest neighbor index method to return the index of closest point rather than the distance itself. 3. Change the me 10000 tric dimension of ambient space encoding a manifold if the metric is of type SRV Metric. 4. Adding a new predict_labels method inside Riemannian Mean Shift class to assign labels to the set of points passed.
Codecov Report
@@ Coverage Diff @@
## master #1525 +/- ##
==========================================
- Coverage 82.88% 82.84% -0.03%
==========================================
Files 96 96
Lines 9750 9763 +13
==========================================
+ Hits 8080 8087 +7
- Misses 1670 1676 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
A new test case for predict_labels in Riemannian Mean Shift method
|
@ninamiolane please review the new test case at your convenience added for maintaining code coverage |
There was a problem hiding this comment.
Sorry I missed it earlier: the tests are failing (see Github actions) because the notebook of cell shapes is failing, which is because the cell shapes have consecutives points there.
Add the preprocess function that removes consecutives points, delete the part of the notebooks that talks about good and bad cells (not needed anymore) and make the notebook pass the tests.
Thanks!
|
@shubhamtalbar96 the multiple checks about NaNs, in the notebook on cell shapes, might not be needed anymore. If they are not needed, could you remove them? Many thanks. |
|
Sure, thank you for the inputs. Will get back shortly. |
Added a new preprocess method to remove duplicate points in a discrete curve.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@ninamiolane can you check the diff in the notebook now? |
There was a problem hiding this comment.
Thanks @shubhamtalbar96! I have some comments that I would like you to address. Probably @ninamiolane can add info in some of them.
@ninamiolane, the notebook on cell shape analysis is breaking because the added check is doing its job: ds_align['jasp']['dlm8'] has overlapping consecutive points.
1. Removed class level tolerance variable 2. Updated metric in Tests for RMS 3. Moved common code to a private function within RMS Test Class
|
The cell shape analysis notebook is still failing. |
|
@shubhamtalbar96 have you performed changes in the cell shape analysis notebook in this PR? For some reason github is not considering it to be a part of "files changed" |
Added a preprocess method to remove duplicate data points in a discrete curve
Added a static method decorator to fix DeepSource issue
|
@LPereira95 apologies the notebook changes were not part of the PR because of an incorrect merge conflict resolution. You should see the changes now in File Changes. |
There was a problem hiding this comment.
Thanks @shubhamtalbar96!
The notebook runs properly now (I haven't made a thorough review there).
Can you address these last (picky) comments, please?
1. Changed the default value of tol in SRV Transform to gs.atol 2. Updated Test Cases in RMS
Vectorized predict_labels method in RMS
|
@shubhamtalbar96 the predict_label function of riemannian_mean_shift is failing, please fix. To see which tests are failing, click on "Details" for the checks that were not successful.
|
| @@ -16,14 +16,13 @@ class TestRiemannianMeanShift(geomstats.tests.TestCase): | |||
| @geomstats.tests.np_autograd_and_torch_only | |||
| def test_hypersphere_riemannian_mean_shift_predict(self): | |||
There was a problem hiding this comment.
here we should remove riemannian_mean_shift from the name of this function: we know that we are in the riemannian_mean_shift test, from the name of the file: test_riemannian_mean_shift.py
@shubhamtalbar96 Could you u EF07 pdate the names of all the tests accordingly? Thanks
|
@ninamiolane the failing of |