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

Check reader for image typing #51

Merged
merged 5 commits into from
Jul 17, 2024
Merged

Conversation

BrianWhitneyAI
Copy link
Contributor

@BrianWhitneyAI BrianWhitneyAI commented Jul 5, 2024

Description

This is a potential solution to our typing discrepancy between a selected Reader and BioImage. This will make it so we do not have to add specific logic to each reader to handle unsupported filetype submission in the condition:

BioImage( ArrayLike, reader = "SOME READER THAT DOESNT SUPPORT ARRAYLIKE")

@BrianWhitneyAI BrianWhitneyAI marked this pull request as ready for review July 5, 2024 20:54
@BrianWhitneyAI BrianWhitneyAI requested a review from a team as a code owner July 5, 2024 20:54
Copy link
Contributor

@toloudis toloudis left a comment

Choose a reason for hiding this comment

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

As I understand it, the goal here is to basically encode the fact that only one Reader makes sense to support ArrayLike things and all the rest should be loading images from urls. I wonder if there is a more proper place in bioio-base to add that check, maybe at the base reader level. Or even have some code or flag or member variable in the ArrayLike reader that indicates that it's special. Just thinking out loud here...

I also wonder if there is a unit test you can write to test this change - do you need to make a mock reader or something like that?

@@ -126,6 +126,37 @@ class BioImage(biob.image_container.ImageContainer):
by either instantiating the reader independently or using the `.reader` property.
"""

@staticmethod
def check_type(obj: Any, expected_type: Type) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

This function looks like it should live in plugins.py or some utility module but not in bioimage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in 408f426

# Init reader
# check specific reader image types in a situation where a specified reader
# only supports some of the ImageLike types.
if not BioImage.check_type(image, reader.__init__.__annotations__["image"]):
Copy link
Contributor

Choose a reason for hiding this comment

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

annotations has different recommended ways of accessing it for python 39 and before, vs 3.10 and up. https://docs.python.org/3/howto/annotations.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I'm not really sure I understand how this works. Can you point to something in bioio-base or one of the readers where this "image" annotation is defined/required? It's hard to tell from just looking here (at least for me).

Copy link
Contributor Author

@BrianWhitneyAI BrianWhitneyAI Jul 10, 2024

Choose a reason for hiding this comment

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

The idea here was to pull the typing of the image parameter from the specified reader after it is selected and make sure they line up. An example of this would be in the ometiff reader (I think just all the readers) here where we define image as pathlike.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

annotations has different recommended ways of accessing it for python 39 and before, vs 3.10 and up. https://docs.python.org/3/howto/annotations.html

I might be missing something here. In looking at these documentations I don't believe that annotations directly apply to what we are doing. This is trying to access the typing of the init of the image whereas the examples seem to be referencing a annotation set internally by the class.

Copy link
Contributor

Choose a reason for hiding this comment

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

annotations has different recommended ways of accessing it for python 39 and before, vs 3.10 and up. https://docs.python.org/3/howto/annotations.html

I might be missing something here. In looking at these documentations I don't believe that annotations directly apply to what we are doing. This is trying to access the typing of the init of the image whereas the examples seem to be referencing a annotation set internally by the class.

I put that link because your code uses __annotations__ (it says reader.__init__.__annotations__ right?) and the Python page I linked says:
This document is designed to encapsulate the best practices for working with annotations dicts. If you write Python code that examines __annotations__ on Python objects, we encourage you to follow the guidelines described below.

Copy link
Contributor

@toloudis toloudis Jul 12, 2024

Choose a reason for hiding this comment

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

Now I am wondering - I'm not sure we guarantee that all Readers have to have an __init__ constructor function that takes a named parameter called image. Is that guaranteed somewhere?

@BrianWhitneyAI
Copy link
Contributor Author

I also wonder if there is a unit test you can write to test this change - do you need to make a mock reader or something like that?

The Idea I was thinking of is that we would include a test for each of the readers like

def test_wrongtype_param() -> None:
    with pytest.raises(exceptions.UnsupportedFileFormatError):
        test_image = np.random.rand(10, 10)
        BioImage(test_image, reader=Reader)

@toloudis
Copy link
Contributor

I also wonder if there is a unit test you can write to test this change - do you need to make a mock reader or something like that?

The Idea I was thinking of is that we would include a test for each of the readers like

I was kinda wondering if you could just do one single test here in this repo (or in bioio-base) rather than add a test to every Reader, just to save the trouble

Copy link
Contributor

@toloudis toloudis left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

Parameters
----------
image : ImageLike
The image to be checked. It can be a PathLike, ArrayLike, MetaArrayLike,

Choose a reason for hiding this comment

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

can be or must be?

@BrianWhitneyAI BrianWhitneyAI merged commit 6aa1d66 into main Jul 17, 2024
13 checks passed
@BrianWhitneyAI BrianWhitneyAI deleted the feature/backcheck-reader-types branch July 17, 2024 18:47
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.

None yet

3 participants