Skip to content

Add the get_transform functionality to the base ROI #786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
animeshsasan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add the get_transform functionality to the base ROI #786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion movement/roi/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@

from abc import ABC, abstractmethod
from collections.abc import Callable, Hashable, Sequence
from typing import TYPE_CHECKING, Any, Generic, TypeAlias, TypeVar, cast
from typing import TYPE_CHECKING, Any, Generic, Self, TypeAlias, TypeVar, cast

import matplotlib.pyplot as plt
import numpy as np
import shapely
from shapely.coords import CoordinateSequence

from movement.transforms import compute_homography_transform
Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

from movement.utils.broadcasting import broadcastable_method
from movement.utils.vector import compute_signed_angle_2d

Expand Down Expand Up @@ -560,3 +561,35 @@ def plot(
if fig is None or ax is None:
fig, ax = plt.subplots(1, 1)
return self._plot(fig, ax, **matplotlib_kwargs)

def get_transform(self, other: Self) -> np.ndarray:
"""Compute the homography transformation matrix to align with `other`.
Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


Parameters
----------
other : BaseRegionOfInterest
Another region of interest to which this region will be aligned.
It should be of the same type (line or polygon) and have
the same number of defining points.

Returns
-------
np.ndarray
A (3, 3) transformation matrix that aligns this region to
the `other` region.

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.
Comment on lines +581 to +590
Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


"""
return compute_homography_transform(
Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_transform is ever allowed across different geometry types (e.g. a closed polygon vs an open line with the same defining points), _validate_points_shape would 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)

Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_coords

This 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).

np.array(self.coords), np.array(other.coords)
)
6 changes: 6 additions & 0 deletions movement/roi/line.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"""1-dimensional lines of interest."""

from typing import Self

import numpy as np
import shapely
import xarray as xr
Expand Down Expand Up @@ -179,3 +181,7 @@ def compute_angle_to_normal(
),
in_degrees=in_degrees,
)

def get_transform(self, other: Self) -> np.ndarray:
Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this override may be unnecessary.

  • LineOfInterest accepts 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_transform already raises informative errors for the actual failure cases (< 4 points, collinear points, shape mismatches). These are more helpful than a blanket NotImplementedError.

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).

"""Throw error for transformation matrix for lines."""
raise NotImplementedError("Homography is undefined for LineOfInterest")
93 changes: 93 additions & 0 deletions tests/test_unit/test_roi/test_transform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import numpy as np
import pytest

from movement.roi.line import LineOfInterest
from movement.roi.polygon import PolygonOfInterest
from movement.transforms import compute_homography_transform


@pytest.mark.parametrize(
["coords_a", "coords_b"],
[
pytest.param(
np.array([[1, 1], [5, 1], [5, 3], [1, 3]], dtype=np.float32),
np.array([[1, 1], [5, 1], [5, 3], [1, 3]], dtype=np.float32),
id="Identical rectangles (identity transform)",
),
pytest.param(
np.array([[0, 0], [1, 0], [1, 1], [0, 1]], dtype=np.float32),
np.array(
[
[3, -1],
[4.73205081, 0],
[3.98205081, 1.29903811],
[2.25, 0.2990381],
],
dtype=np.float32,
),
id="Rotated and scaled square",
),
],
)
def test_get_transform_happy_path(
Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

coords_a: np.ndarray,
coords_b: np.ndarray,
) -> None:
roi_a = PolygonOfInterest(coords_a)
roi_b = PolygonOfInterest(coords_b)

expected_transform = compute_homography_transform(coords_a, coords_b)

computed_transform = roi_a.get_transform(roi_b)

assert computed_transform.shape == (3, 3)
assert np.allclose(computed_transform, expected_transform, atol=1e-6)


@pytest.mark.parametrize(
["coords_a", "coords_b"],
[
pytest.param(
np.array([[0, 0], [1, 0], [1, 1]], dtype=np.float32),
np.array(
[[0, 0], [1, 0], [2, 0.5], [1, 1], [0, 1]], dtype=np.float32
),
id="Triangle vs Pentagon",
),
pytest.param(
np.array([[0, 0], [1, 0], [1, 1], [0, 1]], dtype=np.float32),
np.array([[0, 0], [1, 0], [1, 1]], dtype=np.float32),
id="Quad vs triangle",
),
],
)
def test_get_transform_mismatched_points_raises(
coords_a: np.ndarray,
coords_b: np.ndarray,
) -> None:
roi_a = PolygonOfInterest(coords_a)
roi_b = PolygonOfInterest(coords_b)

with pytest.raises(ValueError):
roi_a.get_transform(roi_b)


@pytest.mark.parametrize(
["coords_a", "coords_b"],
[
pytest.param(
np.array([[0, 0], [1, 1]], dtype=np.float32),
np.array([[0, 0], [1, 2]], dtype=np.float32),
id="2 lines",
)
],
)
def test_line_of_interest_raises(
Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

@niksirbi niksirbi Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

coords_a: np.ndarray,
coords_b: np.ndarray,
):
line_1 = LineOfInterest(points=coords_a)
line_2 = LineOfInterest(points=coords_b)

with pytest.raises(NotImplementedError):
line_1.get_transform(line_2)
Loading