Skip to content

Feature/allow setting resolution level #10

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

Merged
merged 5 commits into from
Dec 6, 2023
Merged

Feature/allow setting resolution level #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ea6a0c
Add getter/setter for resolution level
SeanLeRoy Dec 4, 2023
a4efc0d
Make resolution level usage closer to scenes
SeanLeRoy Dec 4, 2023
5abddb1
Remove type guard
SeanLeRoy Dec 5, 2023
0a47018
Improve resolution levels desc
SeanLeRoy Dec 5, 2023
c2fbc48
Add optional resolution level checks
SeanLeRoy Dec 5, 2023
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
14 changes: 14 additions & 0 deletions bioio_base/image_container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,20 @@ def current_scene_index(self) -> int:
def set_scene(self, scene_id: Union[str, int]) -> None:
pass

@property
@abstractmethod
def resolution_levels(self) -> Tuple[int, ...]:
pass

@property
@abstractmethod
def current_resolution_level(self) -> int:
pass

@abstractmethod
def set_resolution_level(self, resolution_level: int) -> None:
pass

@property
@abstractmethod
def xarray_dask_data(self) -> xr.DataArray:
Expand Down
50 changes: 50 additions & 0 deletions bioio_base/reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ class Reader(ImageContainer, ABC):
_metadata: Optional[Any] = None
_scenes: Optional[Tuple[str, ...]] = None
_current_scene_index: int = 0
_current_resolution_level: int = 0
# Do not default because they aren't used by all readers
_fs: AbstractFileSystem
_path: str
Expand Down Expand Up @@ -194,6 +195,28 @@ def current_scene_index(self) -> int:
"""
return self._current_scene_index

@property
def resolution_levels(self) -> Tuple[int, ...]:
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we just assume they are contiguous indices starting from zero? So all this would have to do is return a count. I think the premise here is that they are just indexed, which is different than how scenes are handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's nothing wrong with this I suppose, but it's possibly engineering in more than what is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought was that if we keep it close to how scenes are used the API would be simpler/more consistent

"""
Returns
-------
resolution_levels: Tuple[str, ...]
Return the available resolution levels for the current scene.
By default these are ordered from highest resolution to lowest
resolution.
"""
return (self._current_resolution_level,)

@property
def current_resolution_level(self) -> int:
"""
Returns
-------
resolution_level: int
The current resolution level.
"""
return self._current_resolution_level

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems backward to me to initialize a current resolution level and then build resolution levels from that initialization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might be misunderstanding what you are getting at, but the reason the _current_resolution_level property has to exist in conjunction to a property of similar name (current_resolution_level) is because the former is protected from getting set by users outside of the way we want (as in we don't want _current_resolution_level = 3 and do want set_resolution_level(3)) and the latter provides an access point to users. We could do with just the private _current_resolution_level but generally you don't want to build an API expecting users to have to access a private property.

def _reset_self(self) -> None:
# Reset the data stored in the Reader object
self._xarray_dask_data = None
Expand Down Expand Up @@ -261,6 +284,33 @@ def set_scene(self, scene_id: Union[str, int]) -> None:
f"or integer (for scene index). Provided: {scene_id} ({type(scene_id)}."
)

def set_resolution_level(self, resolution_level: int) -> None:
"""
Set the resolution level.

Parameters
----------
resolution_level: int
The resolution level to access the image at.

Raises
------
IndexError
The provided resolution level is not found in the
available resolution level list.
"""
# Validate resolution level
if resolution_level not in self.resolution_levels:
raise IndexError(
f"Resolution level: '{resolution_level}' "
"is not present in available image resolution levels: "
f"{self.resolution_levels}. Readers are not required by `bioio-base` "
"to support resolution levels and therefore may not have any "
"available besides the default of 0."
)

self._current_resolution_level = resolution_level

@abstractmethod
def _read_delayed(self) -> xr.DataArray:
"""
Expand Down
16 changes: 16 additions & 0 deletions bioio_base/test_utilities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ def run_image_container_checks(
Optional[float], Optional[float], Optional[float]
],
expected_metadata_type: Union[type, Tuple[Union[type, Tuple[Any, ...]], ...]],
set_resolution_level: int = 0,
expected_current_resolution_level: int = 0,
expected_resolution_levels: Tuple[int, ...] = (0,),
) -> ImageContainer:
"""
A general suite of tests to run against readers.
Expand All @@ -82,6 +85,13 @@ def run_image_container_checks(
assert image_container.scenes == expected_scenes
assert image_container.current_scene == expected_current_scene

# Set resolution level
image_container.set_resolution_level(set_resolution_level)

# Check resolution level info
assert image_container.resolution_levels == expected_resolution_levels
assert image_container.current_resolution_level == expected_current_resolution_level

# Check basics
assert image_container.shape == expected_shape
assert image_container.dtype == expected_dtype
Expand Down Expand Up @@ -158,6 +168,9 @@ def run_image_file_checks(
Optional[float], Optional[float], Optional[float]
],
expected_metadata_type: Union[type, Tuple[Union[type, Tuple[Any, ...]], ...]],
set_resolution_level: int = 0,
expected_current_resolution_level: int = 0,
expected_resolution_levels: Tuple[int, ...] = (0,),
) -> ImageContainer:
# Init container
image_container = ImageContainer(image, fs_kwargs=dict(anon=True))
Expand All @@ -169,8 +182,11 @@ def run_image_file_checks(
run_image_container_checks(
image_container=image_container,
set_scene=set_scene,
set_resolution_level=set_resolution_level,
expected_scenes=expected_scenes,
expected_current_scene=expected_current_scene,
expected_resolution_levels=expected_resolution_levels,
expected_current_resolution_level=expected_current_resolution_level,
expected_shape=expected_shape,
expected_dtype=expected_dtype,
expected_dims_order=expected_dims_order,
Expand Down