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

Changes from 2 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
54 changes: 54 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,26 @@ 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[int, ...]
A tuple of valid resolution levels in the file.
"""
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 +282,39 @@ 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
------
TypeError
The provided value wasn't an integer.
"""
# Ensure typing
if not isinstance(resolution_level, int):
raise TypeError(
f"Must provide either an integer for resolution level "
f". Provided: {resolution_level} ({type(resolution_level)}."
)

# 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