Skip to content
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.

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

Experimenting with shapely RoIs #390

Closed
wants to merge 21 commits into from
Closed

Conversation

willGraham01
Copy link
Contributor

@willGraham01 willGraham01 commented Jan 27, 2025

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

A first step towards addressing #377 - introduces the low-level classes that we will need to represent these in the codebase.

As stated on the issue, we have to use wrapper classes since inheriting from shapely classes is problematic. This doesn't really matter too much though - if anything, it is rather helpful for separating the underlying (and rather counter-intuitive at times) shapely objects from the higher-level analysis functions that we're going to implement.

What does this PR do?

  • Introduces the roi submodule, which contains a base class and two derived classes for representing RoIs.
  • Introduces the make_broadcastable decorator into the utils folder. More on this decorator below.

make_broadcastable

The data that we will typically be seeing is 2-dimensional in space, but may have multiple other dimensions for time, individuals, etc. shapely is not vectorised, so cannot handling broadcasting a single operation across the corresponding dimension of a numpy array. Furthermore, shapely also largely depends on casting to it's own internal Geometry objects before such methods can be applied.

make_broadcastable is a decorator that is designed to help reduce the code bloat that "vectorising" the shapely operations would need. It turns functions that act on 1D data (notably, the shapely functions we will be using) into functions that can act along a given axis of a DataArray input. As an example, we can look at the points_are_inside method that is provided to regions of interest:

@broadcastable_method(only_broadcastable_along="space") # an alias of `make_broadcastable`
def points_are_inside(
    self,
    /,
    position: ArrayLike,
    include_boundary: bool = True,
) -> bool:
    ...

This method relies on a shapely method, so only handles one spatial coordinate per function call. However, by decorating it with make_broadcastable, we can now call it by providing a DataArray instead, as per the corresponding tests in test/test_unit/test_roi/test_points_are_inside.py

region = PolygonOfInterest(...)
data = xr.DataArray(
    data = # time as rows, spatial dims as columns, ...,
    dims = ...,
    coords = ...,
)

in_region = region.points_are_inside(data)

in_region is of the same shape as data, but with the "space" dimension dropped. The values in in_region are bools, corresponding to the output of points_are inside function as called on each coordinate pair in the "space" dimension of data.

More broadly, make_broadcastable is also quite useful to have available publicly to users; they can write functions that assume they're only operating on one "piece" of data (EG one position) and can then use the decorators to extend this in the manner described above.

References

Relates to #377 (but does not close). Does introduce the ability to determine if a point is inside an RoI though.

How has this PR been tested?

Addition of local tests for both RoI classes and the new decorator.

Is this a breaking change?

Does this PR require an update to the documentation?

Should maybe consider starting an example that illustrates how to use RoIs. Possibly also a developer example to explain the usage of the new make_broadcastable decorator.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

@willGraham01
Copy link
Contributor Author

willGraham01 commented Jan 31, 2025

This PR has definitely scope drifted - mostly my fault because I ended up working on the make_broadcastable functionality when I realised I would need it to write the underlying methods for the two region-of-interest classes, and kept pushing to this branch rather than breaking off!

As such, what I'll do is break up the PR into the following to make reviewing it more manageable:

@willGraham01 willGraham01 force-pushed the wgraham-roi-playground branch from 92ba767 to 3a7d56d Compare January 31, 2025 09:02
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 93.70079% with 8 lines in your changes missing coverage. Please review.

Project coverage is 99.14%. Comparing base (84b245c) to head (3a7d56d).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
movement/roi/base.py 88.13% 7 Missing ⚠️
movement/utils/broadcasting.py 98.07% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #390      +/-   ##
==========================================
- Coverage   99.80%   99.14%   -0.67%     
==========================================
  Files          15       19       +4     
  Lines        1048     1175     +127     
==========================================
+ Hits         1046     1165     +119     
- Misses          2       10       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant