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

Implement WsiAnnotations in C++ #252

Merged
merged 98 commits into from
Sep 26, 2024
Merged

Implement WsiAnnotations in C++ #252

merged 98 commits into from
Sep 26, 2024

Conversation

jonasteuwen
Copy link
Contributor

@jonasteuwen jonasteuwen commented Aug 13, 2024

The WsiAnnotations have been rewritten using boost.Polygon, replacing shapely. The current implementation with shapely has a few issues:

  • complicated dependencies (GEOS)
  • if you subclass shapely geometries and apply operations on them, they return the shapely polygon, and not your subclass.

The proposal is to keep WsiAnnotations around until v1.0.0, add deprecation warnings to the current calls that are not 1-1 compatible with the new SlideAnnotations (available in dlup.annotations_experimental). In particular these features are implemented

  • dlup.geometry.Polygon: should be mostly compatible with shapely.geometry.Polygon for our use case
  • dlup.geometry.Point: a point class, same as above
  • dlup.geometry.Box: shapely doesn't support boxes but we do.

The function SlideAnnotations.read_region() returns an AnnotationRegion object rather than list of objects. You can access the polygons using AnnotationRegion.polygons, points using AnnotationRegion.points and boxes using AnnotationRegion.boxes. The first two behave as expected, but the boxes are not cropped to the read_region, so it is possible the (x, y) coordinate is negative (slightly outside of the read_region). In addition there is a call to_mask() that generates the mask. The evaluation is lazy, so a call to read_region does not execute the pipeline until you access the polygons, points, boxes or mask in the object.

The SlideAnnotations has a few new calls:

  • set_offset: offsets all the annotations, useful in e.g. Halo annotations
  • scale: scales all the annotations in place, useful if you have annotations at lower res and want to rescale to level 0
  • as_dlup_xml and from_dlup_xml: this is our own defined schema which is able to serialise SlideAnnotations completely.
  • reindex_polygons: this sets an index for the to_mask() in the polygons.
  • relabel_polygons: as expected

Lazy evaluation

The library supports lazy evaluation, most operations are only performed when requesting masks or polygons.

WARNING: The operations might not all be thread-safe due to their in-place modification behaviour.

Something that might be nice to have

  • Mask support?
  • JSON loading in C++? For a typically annotation it is now ±100ms for the current version, 50ms for the C++ version, and likely that can go down to about 10ms if using C++ JSON parsing.

Please add more possible features.

@jonasteuwen jonasteuwen marked this pull request as ready for review August 13, 2024 22:17
@jonasteuwen jonasteuwen marked this pull request as draft August 13, 2024 22:17
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 83.61757% with 317 lines in your changes missing coverage. Please review.

Project coverage is 85.73%. Comparing base (8d31221) to head (0009ba5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
dlup/annotations_experimental.py 67.10% 131 Missing and 71 partials ⚠️
dlup/utils/geometry_xml.py 50.49% 41 Missing and 9 partials ⚠️
tests/test_slide_annotations.py 90.35% 24 Missing and 6 partials ⚠️
dlup/geometry.py 93.95% 9 Missing and 8 partials ⚠️
dlup/annotations.py 78.43% 8 Missing and 3 partials ⚠️
tests/test_geometry.py 98.34% 6 Missing ⚠️
tests/test_annotations.py 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #252       +/-   ##
===========================================
- Coverage   98.63%   85.73%   -12.90%     
===========================================
  Files          14       45       +31     
  Lines        1169     4985     +3816     
  Branches        0     1074     +1074     
===========================================
+ Hits         1153     4274     +3121     
- Misses         16      490      +474     
- Partials        0      221      +221     

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

@JorenB JorenB self-requested a review September 2, 2024 12:54
Copy link
Contributor

@JorenB JorenB left a comment

Choose a reason for hiding this comment

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

LGTM. All geometry tests pass and the logic is clear.

@jonasteuwen jonasteuwen merged commit a497eab into main Sep 26, 2024
7 of 9 checks passed
@jonasteuwen jonasteuwen deleted the feature/geometry branch September 26, 2024 08:49
jonasteuwen added a commit that referenced this pull request Sep 26, 2024
jonasteuwen added a commit that referenced this pull request Sep 26, 2024
- Add DLUP XML output
- Rewrite setup to use meson-py

---------

Co-authored-by: Bart de Rooij <[email protected]>
Co-authored-by: Jonas Teuwen <[email protected]>
Co-authored-by: JorenB <[email protected]>
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.

4 participants