Skip to content

add thumbnail_size property #34

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

Merged
merged 7 commits into from
Dec 8, 2020
Merged

add thumbnail_size property #34

merged 7 commits into from
Dec 8, 2020

Conversation

emmanuelle
Copy link
Contributor

@emmanuelle emmanuelle commented Dec 3, 2020

[Updated]

This PR introduces a thumbnail_size property of the slicer, in order to be able to choose the size of the low-resolution images uploaded and available clientside. It is an integer, or None, in which case the full resolution data is uploaded clientside (I implemented this by not attaching the server callback, which was an easy solution).

@emmanuelle emmanuelle changed the title add thumbnail_size property [WIP] add thumbnail_size property Dec 3, 2020
@almarklein
Copy link
Collaborator

Also it could be a good idea to let thumbnail_size be a tuple instead of an integer.

I'm not sure if Pillow allows this. Pillow seems to consider the given size as a hint. Which is why we have get_thumbnail_size(). But we can try.

Another nice addition would be to select a better default size when the data is very anisotropic. I noticed too that with the xray app the low-res data is way too low-res :P

BTW: I suspect that we're using different versions of Black (CI runs the latest).

@emmanuelle emmanuelle changed the title [WIP] add thumbnail_size property add thumbnail_size property Dec 4, 2020
@emmanuelle
Copy link
Contributor Author

Ready for review. I tested this on the xray app of the gallery. Probably I should add more tests and an example :-).

Comment on lines 33 to 35
thumbnail_size (int or None): linear size of low-resolution data to be
uploaded to the client. If ``None``, the full-resolution data are
uploaded client-side.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feeling a bit uncomfortable about the None which is often used to denote a default value. What about just thumbnail, which can be set to True/False, but also allowing an int with the size? Once we've fixed the anisotropy thing, the default size should be fine in nearly all cases.

Let's also mention the default size here.

Comment on lines 150 to 151
@property
def thumbnail_size(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if we need a public prop for this. Or do you have a use-case in mind?

Comment on lines 278 to 281
if self._thumbnail_size is None:
thumbnail_size = None
info["lowres_size"] = info["size"]
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a check here to make thumbnail_size None when the given size is larger than the image size?

@almarklein
Copy link
Collaborator

This is nice!

@emmanuelle
Copy link
Contributor Author

Thanks for the review @almarklein !

@almarklein
Copy link
Collaborator

This PR looks good to go. Do you know what's up with the circleci build, @emmanuelle ?

@almarklein almarklein merged commit 2c6eac5 into main Dec 8, 2020
@almarklein almarklein deleted the thumbnail branch December 8, 2020 14:40
@almarklein almarklein mentioned this pull request Dec 11, 2020
Closed
16 tasks
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.

2 participants