Add ClosedDiscreteCurvesStartingAtOrigin #1953
Conversation
ete_curves.py file. This class is used to represent closed discrete curves.
|
Unfortunately, my description wasn't included in the PR. Sorry about that. Let me add it down here: The main contribution is to add the class ClosedDscreteCurvesStartingAtOrigin to the discrete_curves.py file. In my understanding, inthe closed curves case, the SRV transform as an extra point of information and hence I changed a little bit the SRV transform part as well to take this into account. @luisfpereira @alebrigant @ninamiolane Please let me know what you think. |
There was a problem hiding this comment.
Thanks for this PR ! I think the tests are not passing so these are just preliminary comments, waiting for other modifs to make the tests pass.
A general comment on the structure: maybe it would be best to have 3 classes: OpenDiscreteCurvesStartingAtOrigin and ClosedDiscreteCurvesStartingAtOrigin and DiscreteCurvesSartingAtOrigin would call one or the other depending on the attribute self.closed ? It also seems unnecessary to copy a big portion of the code of open curves into the class of closed curves, maybe there should be some inheritance there. What do you think @luisfpereira ? In any case, there is still some work to make the tests pass !
| self._quotient_map = { | ||
| (SRVMetric, "rotations"): ( | ||
| SRVRotationBundle, | ||
| SRVRotationQuotientMetric, |
There was a problem hiding this comment.
This is no longer the right way to define the quotient maps, I believe we no longer have new classes for each quotient metric. See how it is defined for (open) discrete curves.
| @@ -204,6 +223,7 @@ class DiscreteCurvesStartingAtOrigin(NFoldManifold): | |||
|
|
|||
| def __init__(self, ambient_dim=2, k_sampling_points=10, equip=True): | |||
| ambient_manifold = Euclidean(ambient_dim) | |||
| self.closed = False | |||
There was a problem hiding this comment.
Shouldn't this be used to call ClosedDiscreteCurvesStartingAtOrigin if True ?
| point = self._sphere.metric.geodesic( | ||
| initial_point, initial_tangent_vec=initial_tangent_vec | ||
| )(sampling_times) | ||
| return self.projection(point) |
There was a problem hiding this comment.
Does this really generate a curve that "looks" closed ? It seems like this is the same code as the one used for open curves.
| if self.space.closed: | ||
| base_point_with_origin = insert_starting_point_at_end( | ||
| base_point_with_origin | ||
| ) |
There was a problem hiding this comment.
If this increases the number of sampling points of one, you may need to change some normalizations constants that depend on this number of sampling points.
|
|
||
| dt = 1 / (self.k_sampling_points - 1) | ||
| image_point_norm = self.space.ambient_manifold.metric.norm(image_point) | ||
| dt = 1 / (self.space.k_sampling_points - 1) |
There was a problem hiding this comment.
This might need to be updated, see previous comment.
luisfpereira
changed the title
|
@MMalikT, I've done some changes in the PR, to make everything consistent, though the results are disappointing (i.e. curves open when taking geodesics). In particular:
Please let me know what you think. |
Checklist
Description
We add the ClosedDiscreteCurves class back into the discrete_curves.py file. This led to some changes in the SRV transform. More precisely, considering a closed curve, there is an additional derivative vector at the last sampling point.
The goal will be to improve the data analysis in the cancer cell notebook, incoporating the geometry of the cell being closed. @luisfpereira @alebrigant @ninamiolane.
At the moment, the implimentation is not optimal since we won't need to implement a whole new class to distinguish between closed and open curves - probably an argument in form of a boolean will suffice.
Issue
Additional context