-
Notifications
You must be signed in to change notification settings - Fork 135
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
Add image selector #238
Add image selector #238
Conversation
✅ Deploy Preview for phenomenal-crepe-0effec ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
||
@extra | ||
def image_selector( | ||
image: Image.Image, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggesiton: It would be nice to make this a bit more flexible, and handle image strings as well as Image objects. I think that would make people a lot more likely to use it, and it would be easy to just go ahead and convert to a PIL image as-needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is great! I would love to also see support for string image paths, ideally either URLs or local paths -- you could even borrow code from https://github.com/blackary/streamlit-image-coordinates/blob/main/src/streamlit_image_coordinates/__init__.py#L53 if you want.
But, even as-is, it's an excellent extra!
selection (dict): Selection coordinates, output of `image_selector` | ||
""" | ||
|
||
image_array = np.array(image) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to convert to Image and then array? Otherwise a string will fail.
dict: Selection coordinates | ||
""" | ||
|
||
image = convert_to_pil_image(image) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: My preference is to avoid overloading a variable name if it changes type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, one nit and one bug (I think) in the show_selection function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the show_image function if you actually want it to work with strings. Didn't mean to approve :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New extra!