Fix discrete curves vectorization and remove empty quotient metrics #1935
Conversation
…ectorization; remove empty quotient metrics; improve discrete curves testing
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1935 +/- ##
==========================================
- Coverage 90.51% 86.76% -3.74%
==========================================
Files 143 137 -6
Lines 13495 12898 -597
==========================================
- Hits 12214 11190 -1024
- Misses 1281 1708 +427
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -1314,6 +1314,9 @@ def discrete_horizontal_geodesic( | |||
| if initial_point.ndim != end_point.ndim: | |||
| initial_point, end_point = gs.broadcast_arrays(initial_point, end_point) | |||
|
|
|||
| if callable(end_spline): | |||
There was a problem hiding this comment.
Isn't this the same condition as not is_batch ?
There was a problem hiding this comment.
So the issue is that the check_is_batch function in align is only considering point, i.e. for the use case 1 point, multiple base_point it creates only one spline instead of a list, which breaks the for within discrete_horizontal_geodesic.
I could have fixed it in align, but I thought it is not worthy to create multiple equal splines. Therefore, with the callable check we ensure the code works properly for the use case I mentioned above.
Maybe to make the code more readable we can change align and keep callable
if point.ndim == bundle.total_space.point_ndim:
spline = bundle.total_space.interpolate(point)
else:
spline = [bundle.total_space.interpolate(point_) for point_ in point]or change align and remove callable
if point.ndim == bundle.total_space.point_ndim:
spline = bundle.total_space.interpolate(point)
if base_point.ndim > bundle.total_space.point_ndim:
spline = [spline]*base_point.shape[0]
else:
spline = [bundle.total_space.interpolate(point_) for point_ in point]Which solution do you prefer?
There was a problem hiding this comment.
Ah I see ! I think I like the second solution better because it's easier to understand when reading the code.
| (random.randint(2, 3), random.choice([5, 7, 9])), | ||
| ], | ||
| ) | ||
| def srv_reparameterization_bundles(request): |
There was a problem hiding this comment.
Why is this necessary, and no similar function is defined for the other bundles, e.g. the rotation-reparametrization bundle ? I have to admit that for me, this code and the way it is used in the next class, is hard to read.
There was a problem hiding this comment.
So this is a pytest trick to reduce the verbosity of tests. For each param in params, it creates a test instance for TestSRVReparametrizationBundle.
More concretely, here we are parameterizing over ambient_dim and k_sampling_points. In each run of the tests we create two random parameter choices, one with even k_sampling_points, and another with odd k_sampling_points (with random ambient_dim: 2 or 3). It runs all the tests in TestSRVReparametrizationBundle twice, one for each parameter combination.
Without using fixtures, we would have to create e.g. TestSRVReparametrizationBundleOddKSamplingPoints, TestSRVReparametrizationBundleEvenKSamplingPoints, which would differ only on the integer we pass to k_sampling_points.
This also justifies the introduction of TestCase notion: we can use the same tests for different parameter combinations, even without the use of the pytest parameterization feature, just by inheriting from the appropriate TestCase child.
(Writing about all this is definitely in my todo list.)
| @@ -1314,6 +1314,9 @@ def discrete_horizontal_geodesic( | |||
| if initial_point.ndim != end_point.ndim: | |||
| initial_point, end_point = gs.broadcast_arrays(initial_point, end_point) | |||
|
|
|||
| if callable(end_spline): | |||
There was a problem hiding this comment.
Ah I see ! I think I like the second solution better because it's easier to understand when reading the code.
This PR follows #1924 to:
fix
IterativeHorizontalGeodesicAligner.discrete_horizontal_geodesicvectorizationraise
NotImplementedErrorin new fiber bundles to avoid recursion errors when calling the log on the quotient metricsimprove SRVReparametrizationBundle tests:
k_sampling_pointsdue to different behavior in Euler step (this should probably be done in a specific test for the aligner later)improve discrete curves tests by adding tests for all quotient metrics
remove empty quotient metrics (closes On the creation of empty quotient metrics #1931)