Add the get_transform functionality to the base ROI#786
Add the get_transform functionality to the base ROI#786animeshsasan wants to merge 1 commit intoneuroinformatics-unit:mainfrom
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #786 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 34 34
Lines 2111 2117 +6
=========================================
+ Hits 2111 2117 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
niksirbi
left a comment
niksirbi
left a comment
There was a problem hiding this comment.
Thanks for the PR @animeshsasan!
I have two main suggestions. See inline comments for detailed explanations and more minor comments.
- I think the
LineOfInterest.get_transformoverride leading to aNotImplementedErroris unnecessary. We should let the base class also handle transforms for lines (it would actually work for poly-lines with >=4 non-collinear points). This means you should include lines in tests. - For closed geometries (polygons and looping lines),
coordsincludes the closing point (first vertex is repeated), while open geometries (non-looping lines) the number of coords equals the number of unique vertices. I think we should handle this before we pass the coords tocompute_homography_transform(even though the latter drops duplicates).
With these changes, I'll be happy to approve this PR.
Are there plans for an apply_transform method in a follow-up PR?
| return self._plot(fig, ax, **matplotlib_kwargs) | ||
|
|
||
| def get_transform(self, other: Self) -> np.ndarray: | ||
| """Compute the homography transformation matrix to align with `other`. |
There was a problem hiding this comment.
I think a better name for other would be target, since it automatically signals that it serves as the registration target. Also, if you want docstring text to appear as monospace, you should use double backticks (because docstrings get parsed as rst, not md).
| Raises | ||
| ------ | ||
| ValueError | ||
| If the number of coordinate points does not match | ||
| the number of coordinate points in the other region, | ||
| or if there are insufficient points to | ||
| compute the transformation, | ||
| or if the points are not 2-dimensional, | ||
| or if the points are degenerate or collinear, | ||
| making it impossible to compute a valid homography. |
There was a problem hiding this comment.
These errors are actually raised by the underlying function. So, I'd suggest getting rid of the "Raises" section here, and instead include a "See also" section as shown below:
See Also
--------
movement.transforms.compute_homography_transform
The underlying function used for computing the transform.| in_degrees=in_degrees, | ||
| ) | ||
|
|
||
| def get_transform(self, other: Self) -> np.ndarray: |
There was a problem hiding this comment.
I think this override may be unnecessary.
LineOfInterestaccepts 2+ points. A polyline with 4+ non-collinear points has everything needed for a valid homography.- Homography operates on point correspondences — it doesn't care whether the points form a closed polygon or an open polyline. It is not mathematically "undefined" for lines.
- The underlying
compute_homography_transformalready raises informative errors for the actual failure cases (< 4 points, collinear points, shape mismatches). These are more helpful than a blanketNotImplementedError.
Consider removing the override entirely and letting the base class implementation handle lines. Perhaps we should just modify the docstring in the base class implementation to note that get_transform requires at least 4 non-collinear points, regardless of the geometry type (line or polygon).
| ) | ||
| ], | ||
| ) | ||
| def test_line_of_interest_raises( |
There was a problem hiding this comment.
This test uses @pytest.mark.parametrize with a single pytest.param. We are fans of parametrisation, but in this case this adds boilerplate for no benefit. Either define the coords directly in the test body, or test more cases (I don't think you need to, since the underlying function is already well-tested.)
There was a problem hiding this comment.
THis test is also missing the -> None return annotation. We don't strictly insist on type annotation in tests, but here I would add it for consistency.
| import shapely | ||
| from shapely.coords import CoordinateSequence | ||
|
|
||
| from movement.transforms import compute_homography_transform |
There was a problem hiding this comment.
This made me realise that every time we instantiate a RegionOfInterest object, we would be importing the heavy cv2 dependency, even for users that don't need the transform utility.
I suggest moving the cv2 import from the top of movement/transforms.py to inside the compute_homography_transform() function. This way we'll only pull cv2 when the function is actually called.
| making it impossible to compute a valid homography. | ||
|
|
||
| """ | ||
| return compute_homography_transform( |
There was a problem hiding this comment.
The implementation passes np.array(self.coords) to compute_homography_transform. For polygons, shapely's exterior.coords includes the closing point (first vertex repeated), so a 4-vertex polygon yields a (5, 2) array, not (4, 2).
This is not a bug — _filter_invalid_points in transforms.py already detects and drops duplicate points, so the result is correct by design. However, stripping the closing point before calling the function would be cleaner:
- It avoids passing redundant data through the filtering pipeline.
- If
get_transformis ever allowed across different geometry types (e.g. a closed polygon vs an open line with the same defining points),_validate_points_shapewould reject the shape mismatch (5 vs 4) before filtering gets a chance to drop the duplicate. Stripping beforehand would prevent this.
I suggest stripping the last coord for closed geometries (those that loop back to the first point, like polygons or closed lines):
src = np.array(self.coords)
dst = np.array(target.coords)
if self.is_closed:
src = src[:-1]
if target.is_closed:
dst = dst[:-1]
return compute_homography_transform(src, dst)There was a problem hiding this comment.
If you adopt this suggestion alongside my other comment about letting the base class also handle poly-line transforms, you should update the tests to include both lines and polygons.
There was a problem hiding this comment.
An alternative solution would be to define an extra property, called vertices or unique_coords, that would always exclude the 'loop-back' coord, and then use this property inside get_transform.
@property
def vertices(self) -> np.ndarray:
coords = np.array(self.coords)
if self.is_closed:
unique_coords = coords[:-1]
return unique_coordsThis solution may be worth it if we want to keep using vertices in other methods, for example in an apply_transform method (which I assume is next).
| ), | ||
| ], | ||
| ) | ||
| def test_get_transform_happy_path( |
There was a problem hiding this comment.
I would add simple docstring to the tests, with some explanation of what they are doing.
For example, this tests that get_transform correctly delegates to compute_honography_transform
2 participants
Description
What is this PR
Why is this PR needed?
In multi-session or multi-video experiments of the same setup, small shifts in the camera or setup can cause coordinate systems to differ between sessions. This prevents direct comparison or aggregation of tracked positional data across sessions. Even minor changes in angle, distance, or tilt can introduce perspective distortions that make one-to-one point correspondence unreliable without geometric alignment.
What does this PR do?
This PR uses the functionality added in PR-696 to create transformations between the sessions for the RegionOfInterest (RoI) class.
References
#565
#696
How has this PR been tested?
This code was tested using multiple dummy problems in local. Some of the toy problems are also available in the unit tests.
Is this a breaking change?
No
Does this PR require an update to the documentation?
Yes. Documentation for the functions have been added.
Checklist: