✨ Define SemanticSegmentor with the New EngineABC
#866
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev-define-engines-abc #866 +/- ##
==========================================================
+ Coverage 91.19% 94.58% +3.38%
==========================================================
Files 73 73
Lines 9374 9234 -140
Branches 1230 1209 -21
==========================================================
+ Hits 8549 8734 +185
+ Misses 792 468 -324
+ Partials 33 32 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
- Use `input_resolutions` instead of resolution to make engines outputs compatible with ioconfig. - Uses input resolution as a list of dictionaries on units and resolution.
- Use `input_resolutions` instead of resolution to make engines outputs compatible with ioconfig. - Uses input resolution as a list of dictionaries on units and resolution.
…mentor # Conflicts: # tests/engines/test_engine_abc.py # tests/engines/test_patch_predictor.py # tiatoolbox/models/engine/engine_abc.py # tiatoolbox/models/engine/io_config.py # tiatoolbox/models/engine/patch_predictor.py
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a comprehensive refactor of the TIAToolbox engine system, introducing a new abstract base class EngineABC and implementing SemanticSegmentor as an extension of the PatchPredictor. The refactor modernizes the codebase with improved memory management, Dask array integration, and better separation of concerns.
Key changes include:
- New
EngineABCbase class providing unified interface for deep learning engines - Complete rewrite of
SemanticSegmentorextendingPatchPredictorwith WSI-specific functionality - Integration of Dask arrays for memory-efficient processing and caching
- Enhanced error handling and validation with new exception types
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tiatoolbox/utils/transforms.py | Added int type annotation to interpolation parameter |
| tiatoolbox/utils/misc.py | Enhanced utility functions with Dask integration, memory optimization, and new helper functions |
| tiatoolbox/utils/exceptions.py | Added DimensionMismatchError exception class |
| tiatoolbox/models/models_abc.py | Updated abstract method signatures for improved type safety |
| tiatoolbox/models/engine/semantic_segmentor.py | Complete rewrite implementing new EngineABC architecture |
| tiatoolbox/models/engine/patch_predictor.py | Refactored to extend EngineABC with simplified interface |
| tiatoolbox/models/engine/engine_abc.py | New abstract base class for all TIAToolbox engines |
| tiatoolbox/models/dataset/dataset_abc.py | Enhanced dataset classes with output location tracking and validation |
Comments suppressed due to low confidence (1)
tiatoolbox/models/engine/semantic_segmentor.py:535
- Similar to the previous issue,
self.dataloadershould bedataloaderin the else clause on the following line.
canvas, count, canvas_zarr, count_zarr
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| mode: str, | ||
| ioconfig: IOSegmentorConfig, | ||
| save_dir: str | Path, | ||
| images: list[os.PathLike | Path | WSIReader] | np.ndarray, |
There was a problem hiding this comment.
os.Pathlike should include str, and example at top of file illustrates usage using string. But string results in error because code uses things like: save_dir / (get_path(image).stem + suffix) without any conversion into Path.
probably need to make get_path return a Path
| keys_to_compute = [k for k in keys_to_compute if k not in zarr_group] | ||
| write_tasks = [] | ||
| for key in keys_to_compute: | ||
| dask_array = processed_predictions[key].rechunk("auto") |
There was a problem hiding this comment.
Leaving the rechunk to auto doesn't always work.
I ran semantic segmentor tissue mask on a random slide, this line ended up with a dask_array with:
chunks = ((1862, 1800, 1800, 1800, 1800, 450), (13562,))
zarr needs to have even chunks (except for the last one).
So for example, specifying dask_array = processed_predictions[key].rechunk((1800,-1)) here instead made the to_zarr work (results in chunks = ((1800, 1800, 1800, 1800, 1800, 512), (13562,))) which zarr is fine with.
Probably this passes during tests cause the tests use slides with nice round dimensions (like 4k x 4k).
There was a problem hiding this comment.
Todo: helper function to calculate zarr chunks.
# Conflicts: # tests/models/test_dataset.py
3 participants
Summary of Changes
Major Additions
Dask Integration:
daskas a dependency and integrated Dask arrays and lazy computation throughout the engine and patch predictor code.Zarr Output Support:
SemanticSegmentor Engine:
SemanticSegmentorengine with Dask/Zarr support and new test coverage (test_semantic_segmentor.py).semantic_segmentorand removed the oldsemantic_segmentCLI.Enhanced CLI and Config:
Utilities and Validation:
DimensionMismatchError).Changes to
kwargmemory-thresholdnum-loader-workersandnum-postproc-workersintonum-workerscache_modeas cache mode is automatically handled.Major Removals/Refactors
Removed Old CLI and Redundant Code:
semantic_segment.pyCLI and replaced it withsemantic_segmentor.py.Refactored Model and Dataset APIs:
Test Cleanup:
API Consistency:
Notable File Changes
New:
tiatoolbox/cli/semantic_segmentor.pytests/engines/test_semantic_segmentor.pyRemoved:
tiatoolbox/cli/semantic_segment.pyHeavily Modified:
engine_abc.py,patch_predictor.py,semantic_segmentor.pyImpact