From f31c8b2d835abd4914fee8945fd9319e4ac53a97 Mon Sep 17 00:00:00 2001 From: Sam Maxwell Date: Wed, 21 Feb 2024 16:54:56 +0000 Subject: [PATCH 01/15] add typing for main interface file Signed-off-by: Sam Maxwell Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 148 +++++++++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 60 deletions(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index 35387171..38198e80 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -25,8 +25,10 @@ from __future__ import annotations -from collections.abc import Mapping from io import BytesIO +from pathlib import Path +from types import TracebackType +from typing import Iterator, Literal, Mapping, Protocol, TypeVar from PIL import Image, ImageCms @@ -59,80 +61,91 @@ PROPERTY_NAME_BOUNDS_HEIGHT = 'openslide.bounds-height' +class _OpenSlideCacheWrapper(Protocol): + _openslide_cache: lowlevel._OpenSlideCache + + class AbstractSlide: """The base class of a slide object.""" - def __init__(self): + def __init__(self) -> None: self._profile = None - def __enter__(self): + def __enter__(self) -> AbstractSlide: return self - def __exit__(self, exc_type, exc_val, exc_tb): + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: TracebackType | None, + ) -> Literal[False]: self.close() return False @classmethod - def detect_format(cls, filename): + def detect_format(cls, filename: Path | str) -> str | None: """Return a string describing the format of the specified file. If the file format is not recognized, return None.""" raise NotImplementedError - def close(self): + def close(self) -> None: """Close the slide.""" raise NotImplementedError @property - def level_count(self): + def level_count(self) -> int: """The number of levels in the image.""" raise NotImplementedError @property - def level_dimensions(self): + def level_dimensions(self) -> tuple[tuple[int, int], ...]: """A list of (width, height) tuples, one for each level of the image. level_dimensions[n] contains the dimensions of level n.""" raise NotImplementedError @property - def dimensions(self): + def dimensions(self) -> tuple[int, int]: """A (width, height) tuple for level 0 of the image.""" return self.level_dimensions[0] @property - def level_downsamples(self): + def level_downsamples(self) -> tuple[float, ...]: """A list of downsampling factors for each level of the image. level_downsample[n] contains the downsample factor of level n.""" raise NotImplementedError @property - def properties(self): + def properties(self) -> Mapping[str, str]: """Metadata about the image. This is a map: property name -> property value.""" raise NotImplementedError @property - def associated_images(self): + def associated_images(self) -> Mapping[str, Image.Image]: """Images associated with this whole-slide image. This is a map: image name -> PIL.Image.""" raise NotImplementedError @property - def color_profile(self): + def color_profile(self) -> ImageCms.ImageCmsProfile | None: """Color profile for the whole-slide image, or None if unavailable.""" if self._profile is None: return None return ImageCms.getOpenProfile(BytesIO(self._profile)) - def get_best_level_for_downsample(self, downsample): + def get_best_level_for_downsample(self, downsample: float) -> int: """Return the best level for displaying the given downsample.""" raise NotImplementedError - def read_region(self, location, level, size): + def read_region( + self, location: tuple[int, int], level: int, size: tuple[int, int] + ) -> Image.Image: """Return a PIL.Image containing the contents of the region. location: (x, y) tuple giving the top left pixel in the level 0 @@ -141,13 +154,13 @@ def read_region(self, location, level, size): size: (width, height) tuple giving the region size.""" raise NotImplementedError - def set_cache(self, cache): + def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: """Use the specified cache to store recently decoded slide tiles. cache: an OpenSlideCache object.""" raise NotImplementedError - def get_thumbnail(self, size): + def get_thumbnail(self, size: tuple[int, int]) -> Image.Image: """Return a PIL.Image containing an RGB thumbnail of the image. size: the maximum size of the thumbnail.""" @@ -178,7 +191,7 @@ class OpenSlide(AbstractSlide): operations on the OpenSlide object, other than close(), will fail. """ - def __init__(self, filename): + def __init__(self, filename: Path | str | bytes) -> None: """Open a whole-slide image.""" AbstractSlide.__init__(self) self._filename = filename @@ -186,27 +199,27 @@ def __init__(self, filename): if lowlevel.read_icc_profile.available: self._profile = lowlevel.read_icc_profile(self._osr) - def __repr__(self): + def __repr__(self) -> str: return f'{self.__class__.__name__}({self._filename!r})' @classmethod - def detect_format(cls, filename): + def detect_format(cls, filename: Path | str) -> str | None: """Return a string describing the format vendor of the specified file. If the file format is not recognized, return None.""" return lowlevel.detect_vendor(str(filename)) - def close(self): + def close(self) -> None: """Close the OpenSlide object.""" lowlevel.close(self._osr) @property - def level_count(self): + def level_count(self) -> int: """The number of levels in the image.""" return lowlevel.get_level_count(self._osr) @property - def level_dimensions(self): + def level_dimensions(self) -> tuple[tuple[int, int], ...]: """A list of (width, height) tuples, one for each level of the image. level_dimensions[n] contains the dimensions of level n.""" @@ -215,7 +228,7 @@ def level_dimensions(self): ) @property - def level_downsamples(self): + def level_downsamples(self) -> tuple[float, ...]: """A list of downsampling factors for each level of the image. level_downsample[n] contains the downsample factor of level n.""" @@ -224,14 +237,14 @@ def level_downsamples(self): ) @property - def properties(self): + def properties(self) -> _OpenSlideMap[str]: """Metadata about the image. This is a map: property name -> property value.""" return _PropertyMap(self._osr) @property - def associated_images(self): + def associated_images(self) -> _OpenSlideMap[Image.Image]: """Images associated with this whole-slide image. This is a map: image name -> PIL.Image. @@ -240,11 +253,13 @@ def associated_images(self): are not premultiplied.""" return _AssociatedImageMap(self._osr, self._profile) - def get_best_level_for_downsample(self, downsample): + def get_best_level_for_downsample(self, downsample: float) -> int: """Return the best level for displaying the given downsample.""" return lowlevel.get_best_level_for_downsample(self._osr, downsample) - def read_region(self, location, level, size): + def read_region( + self, location: tuple[int, int], level: int, size: tuple[int, int] + ) -> Image.Image: """Return a PIL.Image containing the contents of the region. location: (x, y) tuple giving the top left pixel in the level 0 @@ -261,7 +276,7 @@ def read_region(self, location, level, size): region.info['icc_profile'] = self._profile return region - def set_cache(self, cache): + def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: """Use the specified cache to store recently decoded slide tiles. By default, the object has a private cache with a default size. @@ -274,44 +289,47 @@ def set_cache(self, cache): lowlevel.set_cache(self._osr, llcache) -class _OpenSlideMap(Mapping): - def __init__(self, osr): +MapValue = TypeVar('MapValue', str, Image.Image) + + +class _OpenSlideMap(Mapping[str, MapValue]): + def __init__(self, osr: lowlevel._OpenSlide): self._osr = osr - def __repr__(self): + def __repr__(self) -> str: return f'<{self.__class__.__name__} {dict(self)!r}>' - def __len__(self): + def __len__(self) -> int: return len(self._keys()) - def __iter__(self): + def __iter__(self) -> Iterator[str]: return iter(self._keys()) - def _keys(self): + def _keys(self) -> list[str]: # Private method; always returns list. raise NotImplementedError() -class _PropertyMap(_OpenSlideMap): - def _keys(self): +class _PropertyMap(_OpenSlideMap[str]): + def _keys(self) -> list[str]: return lowlevel.get_property_names(self._osr) - def __getitem__(self, key): + def __getitem__(self, key: str) -> str: v = lowlevel.get_property_value(self._osr, key) if v is None: raise KeyError() return v -class _AssociatedImageMap(_OpenSlideMap): - def __init__(self, osr, profile): +class _AssociatedImageMap(_OpenSlideMap[Image.Image]): + def __init__(self, osr: lowlevel._OpenSlide, profile: bytes | None): _OpenSlideMap.__init__(self, osr) self._profile = profile - def _keys(self): + def _keys(self) -> list[str]: return lowlevel.get_associated_image_names(self._osr) - def __getitem__(self, key): + def __getitem__(self, key: str) -> Image.Image: if key not in self._keys(): raise KeyError() image = lowlevel.read_associated_image(self._osr, key) @@ -333,19 +351,19 @@ class OpenSlideCache: each OpenSlide object has its own cache with a default size. """ - def __init__(self, capacity): + def __init__(self, capacity: int): """Create a tile cache with the specified capacity in bytes.""" self._capacity = capacity self._openslide_cache = lowlevel.cache_create(capacity) - def __repr__(self): + def __repr__(self) -> str: return f'{self.__class__.__name__}({self._capacity!r})' class ImageSlide(AbstractSlide): """A wrapper for a PIL.Image that provides the OpenSlide interface.""" - def __init__(self, file): + def __init__(self, file: str | bytes | Path | Image.Image): """Open an image file. file can be a filename or a PIL.Image.""" @@ -359,11 +377,11 @@ def __init__(self, file): self._image = Image.open(file) self._profile = self._image.info.get('icc_profile') - def __repr__(self): + def __repr__(self) -> str: return f'{self.__class__.__name__}({self._file_arg!r})' @classmethod - def detect_format(cls, filename): + def detect_format(cls, filename: str | bytes | Path) -> str | None: """Return a string describing the format of the specified file. If the file format is not recognized, return None.""" @@ -373,51 +391,54 @@ def detect_format(cls, filename): except OSError: return None - def close(self): + def close(self) -> None: """Close the slide object.""" if self._close: self._image.close() self._close = False - self._image = None + # is it necessary to set to None? + self._image = None # type: ignore @property - def level_count(self): + def level_count(self) -> Literal[1]: """The number of levels in the image.""" return 1 @property - def level_dimensions(self): + def level_dimensions(self) -> tuple[tuple[int, int], ...]: """A list of (width, height) tuples, one for each level of the image. level_dimensions[n] contains the dimensions of level n.""" return (self._image.size,) @property - def level_downsamples(self): + def level_downsamples(self) -> tuple[float, ...]: """A list of downsampling factors for each level of the image. level_downsample[n] contains the downsample factor of level n.""" return (1.0,) @property - def properties(self): + def properties(self) -> dict[str, str]: """Metadata about the image. This is a map: property name -> property value.""" return {} @property - def associated_images(self): + def associated_images(self) -> dict[str, Image.Image]: """Images associated with this whole-slide image. This is a map: image name -> PIL.Image.""" return {} - def get_best_level_for_downsample(self, _downsample): + def get_best_level_for_downsample(self, _downsample: float) -> Literal[0]: """Return the best level for displaying the given downsample.""" return 0 - def read_region(self, location, level, size): + def read_region( + self, location: tuple[int, int], level: int, size: tuple[int, int] + ) -> Image.Image: """Return a PIL.Image containing the contents of the region. location: (x, y) tuple giving the top left pixel in the level 0 @@ -444,14 +465,21 @@ def read_region(self, location, level, size): ]: # "< 0" not a typo # Crop size is greater than zero in both dimensions. # PIL thinks the bottom right is the first *excluded* pixel - crop = self._image.crop(image_topleft + [d + 1 for d in image_bottomright]) - tile_offset = tuple(il - l for il, l in zip(image_topleft, location)) + crop = self._image.crop( + ( + image_topleft[0], + image_topleft[1], + image_bottomright[0] + 1, + image_bottomright[1] + 1, + ) + ) + tile_offset = image_topleft[0] - location[0], image_topleft[1] - location[1] tile.paste(crop, tile_offset) if self._profile is not None: tile.info['icc_profile'] = self._profile return tile - def set_cache(self, cache): + def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: """Use the specified cache to store recently decoded slide tiles. ImageSlide does not support caching, so this method does nothing. @@ -460,7 +488,7 @@ def set_cache(self, cache): pass -def open_slide(filename): +def open_slide(filename: str | bytes | Path) -> AbstractSlide: """Open a whole-slide or regular image. Return an OpenSlide object for whole-slide images and an ImageSlide From f4b813141ab23fc9aa17d3cb05df7eca97517117 Mon Sep 17 00:00:00 2001 From: Sam Maxwell Date: Thu, 22 Feb 2024 00:15:48 +0000 Subject: [PATCH 02/15] type the deepzoom generator Signed-off-by: Sam Maxwell --- openslide/deepzoom.py | 93 ++++++++++++++++++++++++++++--------------- 1 file changed, 62 insertions(+), 31 deletions(-) diff --git a/openslide/deepzoom.py b/openslide/deepzoom.py index 66734d70..df802559 100644 --- a/openslide/deepzoom.py +++ b/openslide/deepzoom.py @@ -46,7 +46,13 @@ class DeepZoomGenerator: openslide.PROPERTY_NAME_BOUNDS_HEIGHT, ) - def __init__(self, osr, tile_size=254, overlap=1, limit_bounds=False): + def __init__( + self, + osr: openslide.AbstractSlide, + tile_size: int = 254, + overlap: int = 1, + limit_bounds: bool = False, + ): """Create a DeepZoomGenerator wrapping an OpenSlide object. osr: a slide object. @@ -101,7 +107,7 @@ def __init__(self, osr, tile_size=254, overlap=1, limit_bounds=False): self._z_dimensions = tuple(reversed(z_dimensions)) # Tile - def tiles(z_lim): + def tiles(z_lim: int) -> int: return int(math.ceil(z_lim / self._z_t_downsample)) self._t_dimensions = tuple( @@ -112,7 +118,8 @@ def tiles(z_lim): self._dz_levels = len(self._z_dimensions) # Total downsamples for each Deep Zoom level - l0_z_downsamples = tuple( + # mypy infers this as a tuple[Any, ...] due to the ** operator + l0_z_downsamples: tuple[int, ...] = tuple( 2 ** (self._dz_levels - dz_level - 1) for dz_level in range(self._dz_levels) ) @@ -134,7 +141,7 @@ def tiles(z_lim): openslide.PROPERTY_NAME_BACKGROUND_COLOR, 'ffffff' ) - def __repr__(self): + def __repr__(self) -> str: return '{}({!r}, tile_size={!r}, overlap={!r}, limit_bounds={!r})'.format( self.__class__.__name__, self._osr, @@ -144,26 +151,26 @@ def __repr__(self): ) @property - def level_count(self): + def level_count(self) -> int: """The number of Deep Zoom levels in the image.""" return self._dz_levels @property - def level_tiles(self): + def level_tiles(self) -> tuple[tuple[int, int], ...]: """A list of (tiles_x, tiles_y) tuples for each Deep Zoom level.""" return self._t_dimensions @property - def level_dimensions(self): + def level_dimensions(self) -> tuple[tuple[int, ...], ...]: """A list of (pixels_x, pixels_y) tuples for each Deep Zoom level.""" return self._z_dimensions @property - def tile_count(self): + def tile_count(self) -> int: """The total number of Deep Zoom tiles in the image.""" return sum(t_cols * t_rows for t_cols, t_rows in self._t_dimensions) - def get_tile(self, level, address): + def get_tile(self, level: int, address: tuple[int, int]) -> Image.Image: """Return an RGB PIL.Image for a tile. level: the Deep Zoom level. @@ -191,7 +198,9 @@ def get_tile(self, level, address): return tile - def _get_tile_info(self, dz_level, t_location): + def _get_tile_info( + self, dz_level: int, t_location: tuple[int, int] + ) -> tuple[tuple[tuple[int, int], int, tuple[int, int]], tuple[int, int]]: # Check parameters if dz_level < 0 or dz_level >= self._dz_levels: raise ValueError("Invalid level") @@ -210,42 +219,62 @@ def _get_tile_info(self, dz_level, t_location): ) # Get final size of the tile - z_size = tuple( - min(self._z_t_downsample, z_lim - self._z_t_downsample * t) + z_tl + z_br - for t, z_lim, z_tl, z_br in zip( - t_location, self._z_dimensions[dz_level], z_overlap_tl, z_overlap_br + z_size = ( + min( + self._z_t_downsample, + self._z_dimensions[dz_level][0] - self._z_t_downsample * t_location[0], ) + + z_overlap_tl[0] + + z_overlap_br[0], + min( + self._z_t_downsample, + self._z_dimensions[dz_level][1] - self._z_t_downsample * t_location[1], + ) + + z_overlap_tl[1] + + z_overlap_br[1], ) # Obtain the region coordinates - z_location = [self._z_from_t(t) for t in t_location] - l_location = [ - self._l_from_z(dz_level, z - z_tl) - for z, z_tl in zip(z_location, z_overlap_tl) - ] + z_location = (self._z_from_t(t_location[0]), self._z_from_t(t_location[1])) + l_location = ( + self._l_from_z(dz_level, z_location[0] - z_overlap_tl[0]), + self._l_from_z(dz_level, z_location[1] - z_overlap_tl[1]), + ) # Round location down and size up, and add offset of active area - l0_location = tuple( - int(self._l0_from_l(slide_level, l) + l0_off) - for l, l0_off in zip(l_location, self._l0_offset) + l0_location = ( + int(self._l0_from_l(slide_level, l_location[0]) + self._l0_offset[0]), + int(self._l0_from_l(slide_level, l_location[1]) + self._l0_offset[1]), ) - l_size = tuple( - int(min(math.ceil(self._l_from_z(dz_level, dz)), l_lim - math.ceil(l))) - for l, dz, l_lim in zip(l_location, z_size, self._l_dimensions[slide_level]) + l_size = ( + int( + min( + math.ceil(self._l_from_z(dz_level, z_size[0])), + self._l_dimensions[slide_level][0] - math.ceil(l_location[0]), + ) + ), + int( + min( + math.ceil(self._l_from_z(dz_level, z_size[1])), + self._l_dimensions[slide_level][1] - math.ceil(l_location[1]), + ) + ), ) # Return read_region() parameters plus tile size for final scaling return ((l0_location, slide_level, l_size), z_size) - def _l0_from_l(self, slide_level, l): + def _l0_from_l(self, slide_level: int, l: float) -> float: return self._l0_l_downsamples[slide_level] * l - def _l_from_z(self, dz_level, z): + def _l_from_z(self, dz_level: int, z: int) -> float: return self._l_z_downsamples[dz_level] * z - def _z_from_t(self, t): + def _z_from_t(self, t: int) -> int: return self._z_t_downsample * t - def get_tile_coordinates(self, level, address): + def get_tile_coordinates( + self, level: int, address: tuple[int, int] + ) -> tuple[tuple[int, int], int, tuple[int, int]]: """Return the OpenSlide.read_region() arguments for the specified tile. Most users should call get_tile() rather than calling @@ -256,7 +285,9 @@ def get_tile_coordinates(self, level, address): tuple.""" return self._get_tile_info(level, address)[0] - def get_tile_dimensions(self, level, address): + def get_tile_dimensions( + self, level: int, address: tuple[int, int] + ) -> tuple[int, int]: """Return a (pixels_x, pixels_y) tuple for the specified tile. level: the Deep Zoom level. @@ -264,7 +295,7 @@ def get_tile_dimensions(self, level, address): tuple.""" return self._get_tile_info(level, address)[1] - def get_dzi(self, format): + def get_dzi(self, format: str) -> str: """Return a string containing the XML metadata for the .dzi file. format: the format of the individual tiles ('png' or 'jpeg')""" From 5032e2352e3361467faa07ab80f6c881f5b782aa Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Wed, 16 Oct 2024 08:40:46 -0700 Subject: [PATCH 03/15] Minor type fixes Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index 38198e80..7f8af31b 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -69,7 +69,7 @@ class AbstractSlide: """The base class of a slide object.""" def __init__(self) -> None: - self._profile = None + self._profile: bytes | None = None def __enter__(self) -> AbstractSlide: return self @@ -387,7 +387,9 @@ def detect_format(cls, filename: str | bytes | Path) -> str | None: If the file format is not recognized, return None.""" try: with Image.open(filename) as img: - return img.format + # img currently resolves as Any + # https://github.com/python-pillow/Pillow/pull/8362 + return img.format # type: ignore[no-any-return] except OSError: return None From a6c2f820eabbdeeb55117778891bd2010210acc6 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sun, 6 Oct 2024 18:26:31 -0700 Subject: [PATCH 04/15] Enable type checking for entire openslide package Signed-off-by: Benjamin Gilbert --- .pre-commit-config.yaml | 2 +- pyproject.toml | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 76328b74..6041918a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -59,7 +59,7 @@ repos: - id: mypy name: Check Python types additional_dependencies: [openslide-bin, pillow, types-setuptools] - exclude: "^(doc/.*|openslide/(__init__|deepzoom)\\.py|tests/.*|examples/deepzoom/.*)$" + exclude: "^(doc/.*|tests/.*|examples/deepzoom/.*)$" - repo: https://github.com/rstcheck/rstcheck rev: v6.2.4 diff --git a/pyproject.toml b/pyproject.toml index e0f67df2..8fd7be02 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -60,8 +60,6 @@ force_sort_within_sections = true [tool.mypy] python_version = "3.10" strict = true -# temporary, while we bootstrap type checking -follow_imports = "silent" [tool.pytest.ini_options] minversion = "7.0" From e52cb3a5c5057f1fb984948d13918431da855acd Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sun, 6 Oct 2024 23:15:54 -0700 Subject: [PATCH 05/15] Return subclass from AbstractSlide.__enter__() AbstractSlide subclasses will always return their specific type from this method. Ensure the type system knows this. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index 7f8af31b..521c0f18 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -60,6 +60,8 @@ PROPERTY_NAME_BOUNDS_WIDTH = 'openslide.bounds-width' PROPERTY_NAME_BOUNDS_HEIGHT = 'openslide.bounds-height' +_T = TypeVar('_T') + class _OpenSlideCacheWrapper(Protocol): _openslide_cache: lowlevel._OpenSlideCache @@ -71,7 +73,7 @@ class AbstractSlide: def __init__(self) -> None: self._profile: bytes | None = None - def __enter__(self) -> AbstractSlide: + def __enter__(self: _T) -> _T: return self def __exit__( From 87495baabd5fd40b0eccdf3203ace4417ace6266 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sun, 6 Oct 2024 23:03:55 -0700 Subject: [PATCH 06/15] Disallow bytes in filename arguments bytes will not work in at least one case (the OpenSlide initializer), are not documented to work, and the type annotations are currently inconsistent about accepting it. For now, forbid it everywhere for consistency. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index 521c0f18..a6ebe380 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -86,7 +86,7 @@ def __exit__( return False @classmethod - def detect_format(cls, filename: Path | str) -> str | None: + def detect_format(cls, filename: str | Path) -> str | None: """Return a string describing the format of the specified file. If the file format is not recognized, return None.""" @@ -193,7 +193,7 @@ class OpenSlide(AbstractSlide): operations on the OpenSlide object, other than close(), will fail. """ - def __init__(self, filename: Path | str | bytes) -> None: + def __init__(self, filename: str | Path) -> None: """Open a whole-slide image.""" AbstractSlide.__init__(self) self._filename = filename @@ -205,7 +205,7 @@ def __repr__(self) -> str: return f'{self.__class__.__name__}({self._filename!r})' @classmethod - def detect_format(cls, filename: Path | str) -> str | None: + def detect_format(cls, filename: str | Path) -> str | None: """Return a string describing the format vendor of the specified file. If the file format is not recognized, return None.""" @@ -365,7 +365,7 @@ def __repr__(self) -> str: class ImageSlide(AbstractSlide): """A wrapper for a PIL.Image that provides the OpenSlide interface.""" - def __init__(self, file: str | bytes | Path | Image.Image): + def __init__(self, file: str | Path | Image.Image): """Open an image file. file can be a filename or a PIL.Image.""" @@ -383,7 +383,7 @@ def __repr__(self) -> str: return f'{self.__class__.__name__}({self._file_arg!r})' @classmethod - def detect_format(cls, filename: str | bytes | Path) -> str | None: + def detect_format(cls, filename: str | Path) -> str | None: """Return a string describing the format of the specified file. If the file format is not recognized, return None.""" @@ -492,7 +492,7 @@ def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: pass -def open_slide(filename: str | bytes | Path) -> AbstractSlide: +def open_slide(filename: str | Path) -> AbstractSlide: """Open a whole-slide or regular image. Return an OpenSlide object for whole-slide images and an ImageSlide From 8c2636185927b3e0f51d9791576f45b579a2b9c5 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Fri, 18 Oct 2024 01:33:50 -0700 Subject: [PATCH 07/15] Explicitly hint the possible open_slide() return types open_slide() can't return any possible AbstractSlide subclass, only the two that it's documented to return. Make that explicit in the type hint. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index a6ebe380..a9759f3e 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -492,7 +492,7 @@ def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: pass -def open_slide(filename: str | Path) -> AbstractSlide: +def open_slide(filename: str | Path) -> OpenSlide | ImageSlide: """Open a whole-slide or regular image. Return an OpenSlide object for whole-slide images and an ImageSlide From 076e96fdfcbebea30ffeddd7b25d84bcca8d5aff Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sun, 6 Oct 2024 23:08:32 -0700 Subject: [PATCH 08/15] Loosen mapping type guarantees Avoid overcommitting to internal implementation details in the types being returned. It's sufficient to say that the property and associated image maps implement Mapping. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index a9759f3e..a1056cd1 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -239,14 +239,14 @@ def level_downsamples(self) -> tuple[float, ...]: ) @property - def properties(self) -> _OpenSlideMap[str]: + def properties(self) -> Mapping[str, str]: """Metadata about the image. This is a map: property name -> property value.""" return _PropertyMap(self._osr) @property - def associated_images(self) -> _OpenSlideMap[Image.Image]: + def associated_images(self) -> Mapping[str, Image.Image]: """Images associated with this whole-slide image. This is a map: image name -> PIL.Image. @@ -291,10 +291,7 @@ def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: lowlevel.set_cache(self._osr, llcache) -MapValue = TypeVar('MapValue', str, Image.Image) - - -class _OpenSlideMap(Mapping[str, MapValue]): +class _OpenSlideMap(Mapping[str, _T]): def __init__(self, osr: lowlevel._OpenSlide): self._osr = osr @@ -423,14 +420,14 @@ def level_downsamples(self) -> tuple[float, ...]: return (1.0,) @property - def properties(self) -> dict[str, str]: + def properties(self) -> Mapping[str, str]: """Metadata about the image. This is a map: property name -> property value.""" return {} @property - def associated_images(self) -> dict[str, Image.Image]: + def associated_images(self) -> Mapping[str, Image.Image]: """Images associated with this whole-slide image. This is a map: image name -> PIL.Image.""" From dccce62198666eef48181ec221022a1e5524a8bd Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sun, 6 Oct 2024 22:38:05 -0700 Subject: [PATCH 09/15] Drop _OpenSlideCacheWrapper We don't want to encourage library users to duck-type a replacement for OpenSlideCache, since any replacement would inherently need to make assumptions about our implementation details. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index a1056cd1..be3a9871 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -28,7 +28,7 @@ from io import BytesIO from pathlib import Path from types import TracebackType -from typing import Iterator, Literal, Mapping, Protocol, TypeVar +from typing import Iterator, Literal, Mapping, TypeVar from PIL import Image, ImageCms @@ -63,10 +63,6 @@ _T = TypeVar('_T') -class _OpenSlideCacheWrapper(Protocol): - _openslide_cache: lowlevel._OpenSlideCache - - class AbstractSlide: """The base class of a slide object.""" @@ -156,7 +152,7 @@ def read_region( size: (width, height) tuple giving the region size.""" raise NotImplementedError - def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: + def set_cache(self, cache: OpenSlideCache) -> None: """Use the specified cache to store recently decoded slide tiles. cache: an OpenSlideCache object.""" @@ -278,7 +274,7 @@ def read_region( region.info['icc_profile'] = self._profile return region - def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: + def set_cache(self, cache: OpenSlideCache) -> None: """Use the specified cache to store recently decoded slide tiles. By default, the object has a private cache with a default size. @@ -480,7 +476,7 @@ def read_region( tile.info['icc_profile'] = self._profile return tile - def set_cache(self, cache: _OpenSlideCacheWrapper) -> None: + def set_cache(self, cache: OpenSlideCache) -> None: """Use the specified cache to store recently decoded slide tiles. ImageSlide does not support caching, so this method does nothing. From cfd166361b37d2af89e92e35eac3755a626f72c4 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Thu, 17 Oct 2024 23:10:56 -0700 Subject: [PATCH 10/15] Explicitly check for closed ImageSlide When an operation is performed on a closed ImageSlide, we've historically raised an AttributeError upon dereferencing None. Add proper checks so the type system understands what we're doing, raising ValueError as lowlevel does. This is not an API change because it only affects invalid caller behavior. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 10 +++++++--- tests/test_imageslide.py | 8 ++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index be3a9871..67960d88 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -366,7 +366,7 @@ def __init__(self, file: str | Path | Image.Image): self._file_arg = file if isinstance(file, Image.Image): self._close = False - self._image = file + self._image: Image.Image | None = file else: self._close = True self._image = Image.open(file) @@ -391,10 +391,10 @@ def detect_format(cls, filename: str | Path) -> str | None: def close(self) -> None: """Close the slide object.""" if self._close: + assert self._image is not None self._image.close() self._close = False - # is it necessary to set to None? - self._image = None # type: ignore + self._image = None @property def level_count(self) -> Literal[1]: @@ -406,6 +406,8 @@ def level_dimensions(self) -> tuple[tuple[int, int], ...]: """A list of (width, height) tuples, one for each level of the image. level_dimensions[n] contains the dimensions of level n.""" + if self._image is None: + raise ValueError('Passing closed slide object') return (self._image.size,) @property @@ -442,6 +444,8 @@ def read_region( reference frame. level: the level number. size: (width, height) tuple giving the region size.""" + if self._image is None: + raise ValueError('Passing closed slide object') if level != 0: raise OpenSlideError("Invalid level") if ['fail' for s in size if s < 0]: diff --git a/tests/test_imageslide.py b/tests/test_imageslide.py index 603cb72b..dd00ad66 100644 --- a/tests/test_imageslide.py +++ b/tests/test_imageslide.py @@ -49,8 +49,9 @@ def test_operations_on_closed_handle(self): osr = ImageSlide(img) osr.close() self.assertRaises( - AttributeError, lambda: osr.read_region((0, 0), 0, (100, 100)) + ValueError, lambda: osr.read_region((0, 0), 0, (100, 100)) ) + self.assertRaises(ValueError, lambda: osr.level_dimensions) # If an Image is passed to the constructor, ImageSlide.close() # shouldn't close it self.assertEqual(img.getpixel((0, 0)), 3) @@ -59,9 +60,8 @@ def test_context_manager(self): osr = ImageSlide(file_path('boxes.png')) with osr: pass - self.assertRaises( - AttributeError, lambda: osr.read_region((0, 0), 0, (100, 100)) - ) + self.assertRaises(ValueError, lambda: osr.read_region((0, 0), 0, (100, 100))) + self.assertRaises(ValueError, lambda: osr.level_dimensions) class _SlideTest: From f24c1dd5bf72af8caa284205d2de7735c1b12462 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Fri, 18 Oct 2024 23:43:02 -0700 Subject: [PATCH 11/15] deepzoom: revert unrolling of _get_tile_info() We don't need to avoid generator expressions to keep the type checker happy; we can just assert that the resulting tuples have the correct length. This lets us avoid carrying redundant unrolled code. Signed-off-by: Benjamin Gilbert --- openslide/deepzoom.py | 49 ++++++++++++++----------------------------- 1 file changed, 16 insertions(+), 33 deletions(-) diff --git a/openslide/deepzoom.py b/openslide/deepzoom.py index df802559..c0d1a4af 100644 --- a/openslide/deepzoom.py +++ b/openslide/deepzoom.py @@ -219,48 +219,31 @@ def _get_tile_info( ) # Get final size of the tile - z_size = ( - min( - self._z_t_downsample, - self._z_dimensions[dz_level][0] - self._z_t_downsample * t_location[0], + z_size = tuple( + min(self._z_t_downsample, z_lim - self._z_t_downsample * t) + z_tl + z_br + for t, z_lim, z_tl, z_br in zip( + t_location, self._z_dimensions[dz_level], z_overlap_tl, z_overlap_br ) - + z_overlap_tl[0] - + z_overlap_br[0], - min( - self._z_t_downsample, - self._z_dimensions[dz_level][1] - self._z_t_downsample * t_location[1], - ) - + z_overlap_tl[1] - + z_overlap_br[1], ) # Obtain the region coordinates - z_location = (self._z_from_t(t_location[0]), self._z_from_t(t_location[1])) - l_location = ( - self._l_from_z(dz_level, z_location[0] - z_overlap_tl[0]), - self._l_from_z(dz_level, z_location[1] - z_overlap_tl[1]), - ) + z_location = [self._z_from_t(t) for t in t_location] + l_location = [ + self._l_from_z(dz_level, z - z_tl) + for z, z_tl in zip(z_location, z_overlap_tl) + ] # Round location down and size up, and add offset of active area - l0_location = ( - int(self._l0_from_l(slide_level, l_location[0]) + self._l0_offset[0]), - int(self._l0_from_l(slide_level, l_location[1]) + self._l0_offset[1]), + l0_location = tuple( + int(self._l0_from_l(slide_level, l) + l0_off) + for l, l0_off in zip(l_location, self._l0_offset) ) - l_size = ( - int( - min( - math.ceil(self._l_from_z(dz_level, z_size[0])), - self._l_dimensions[slide_level][0] - math.ceil(l_location[0]), - ) - ), - int( - min( - math.ceil(self._l_from_z(dz_level, z_size[1])), - self._l_dimensions[slide_level][1] - math.ceil(l_location[1]), - ) - ), + l_size = tuple( + int(min(math.ceil(self._l_from_z(dz_level, dz)), l_lim - math.ceil(l))) + for l, dz, l_lim in zip(l_location, z_size, self._l_dimensions[slide_level]) ) # Return read_region() parameters plus tile size for final scaling + assert len(l0_location) == 2 and len(l_size) == 2 and len(z_size) == 2 return ((l0_location, slide_level, l_size), z_size) def _l0_from_l(self, slide_level: int, l: float) -> float: From 0d675d65c11a28e86f1f3e8dfd80a749626cb14f Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sat, 19 Oct 2024 00:29:41 -0700 Subject: [PATCH 12/15] deepzoom: fix level_dimensions type hint level_dimensions is always a tuple of 2-tuples. Prove this to the type checker. Signed-off-by: Benjamin Gilbert --- openslide/deepzoom.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/openslide/deepzoom.py b/openslide/deepzoom.py index c0d1a4af..623e3adb 100644 --- a/openslide/deepzoom.py +++ b/openslide/deepzoom.py @@ -27,12 +27,17 @@ from io import BytesIO import math +from typing import TYPE_CHECKING from xml.etree.ElementTree import Element, ElementTree, SubElement from PIL import Image import openslide +if TYPE_CHECKING: + # Python 3.10+ + from typing import TypeGuard + class DeepZoomGenerator: """Generates Deep Zoom tiles and metadata.""" @@ -104,7 +109,8 @@ def __init__( while z_size[0] > 1 or z_size[1] > 1: z_size = tuple(max(1, int(math.ceil(z / 2))) for z in z_size) z_dimensions.append(z_size) - self._z_dimensions = tuple(reversed(z_dimensions)) + # Narrow the type, for self.level_dimensions + self._z_dimensions = self._pairs_from_n_tuples(tuple(reversed(z_dimensions))) # Tile def tiles(z_lim: int) -> int: @@ -161,7 +167,7 @@ def level_tiles(self) -> tuple[tuple[int, int], ...]: return self._t_dimensions @property - def level_dimensions(self) -> tuple[tuple[int, ...], ...]: + def level_dimensions(self) -> tuple[tuple[int, int], ...]: """A list of (pixels_x, pixels_y) tuples for each Deep Zoom level.""" return self._z_dimensions @@ -255,6 +261,18 @@ def _l_from_z(self, dz_level: int, z: int) -> float: def _z_from_t(self, t: int) -> int: return self._z_t_downsample * t + @staticmethod + def _pairs_from_n_tuples( + tuples: tuple[tuple[int, ...], ...] + ) -> tuple[tuple[int, int], ...]: + def all_pairs( + tuples: tuple[tuple[int, ...], ...] + ) -> TypeGuard[tuple[tuple[int, int], ...]]: + return all(len(t) == 2 for t in tuples) + + assert all_pairs(tuples) + return tuples + def get_tile_coordinates( self, level: int, address: tuple[int, int] ) -> tuple[tuple[int, int], int, tuple[int, int]]: From 981684ba6abdce19cdd41a4f8e2c9b61e7f32aa7 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sat, 19 Oct 2024 05:13:33 -0700 Subject: [PATCH 13/15] Revert unrolling of ImageSlide.read_region() Keep the type checker happy by asserting the correct tuple lengths rather than unrolling generator expressions. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index 67960d88..dc85a630 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -466,15 +466,10 @@ def read_region( ]: # "< 0" not a typo # Crop size is greater than zero in both dimensions. # PIL thinks the bottom right is the first *excluded* pixel - crop = self._image.crop( - ( - image_topleft[0], - image_topleft[1], - image_bottomright[0] + 1, - image_bottomright[1] + 1, - ) - ) - tile_offset = image_topleft[0] - location[0], image_topleft[1] - location[1] + crop_box = tuple(image_topleft + [d + 1 for d in image_bottomright]) + tile_offset = tuple(il - l for il, l in zip(image_topleft, location)) + assert len(crop_box) == 4 and len(tile_offset) == 2 + crop = self._image.crop(crop_box) tile.paste(crop, tile_offset) if self._profile is not None: tile.info['icc_profile'] = self._profile From f1d02fc24f1e942fa85faf23c29f6d14c1d60e4b Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sat, 19 Oct 2024 06:40:03 -0700 Subject: [PATCH 14/15] Drop redundant return type hint __init__() methods don't need a return type hint unless they take no arguments. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index dc85a630..e6999d52 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -189,7 +189,7 @@ class OpenSlide(AbstractSlide): operations on the OpenSlide object, other than close(), will fail. """ - def __init__(self, filename: str | Path) -> None: + def __init__(self, filename: str | Path): """Open a whole-slide image.""" AbstractSlide.__init__(self) self._filename = filename From 0efc25bb440e3cf782a131f3d9ef6875f8df4967 Mon Sep 17 00:00:00 2001 From: Benjamin Gilbert Date: Sat, 19 Oct 2024 06:42:57 -0700 Subject: [PATCH 15/15] Narrow some ImageSlide property types ImageSlide is hardcoded to a single level, and the level_count property and get_best_level_for_downsample() return types already reflect this. Reduce the level_dimensions() and level_downsamples() return types to 1-tuples. In the latter case we can't reduce to a Literal because literal floats are disallowed. Signed-off-by: Benjamin Gilbert --- openslide/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openslide/__init__.py b/openslide/__init__.py index e6999d52..53c70756 100644 --- a/openslide/__init__.py +++ b/openslide/__init__.py @@ -402,7 +402,7 @@ def level_count(self) -> Literal[1]: return 1 @property - def level_dimensions(self) -> tuple[tuple[int, int], ...]: + def level_dimensions(self) -> tuple[tuple[int, int]]: """A list of (width, height) tuples, one for each level of the image. level_dimensions[n] contains the dimensions of level n.""" @@ -411,7 +411,7 @@ def level_dimensions(self) -> tuple[tuple[int, int], ...]: return (self._image.size,) @property - def level_downsamples(self) -> tuple[float, ...]: + def level_downsamples(self) -> tuple[float]: """A list of downsampling factors for each level of the image. level_downsample[n] contains the downsample factor of level n."""