-
Notifications
You must be signed in to change notification settings - Fork 0
update vole-app and demo uses bioio #26
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
Conversation
interim17
left a comment
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.
Didn't run but LGTM
| During development it may sometimes be useful to do some manual cleanup: | ||
|
|
||
| ```bash | ||
| rm -rf node_modules | ||
| rm -rf yarn.lock | ||
| jlpm install | ||
| jlpm build | ||
| ``` |
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 can think of some other repos that could use this
| # mn = min(0, resized_channel.min()) | ||
| # mx = resized_channel.max() | ||
| # resized_channel = 255.0 * (resized_channel - mn) / (mx - mn) | ||
|
|
||
| # resized_channel = resized_channel.astype(numpy.uint8) |
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.
Curious why this is retained?
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.
yeah I suppose we can just delete it
ShrimpCryptid
left a comment
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.
LGTM, left a few questions!
| <ViewerStateProvider> | ||
| <ImageViewerApp {...viewerProps} /> | ||
| </ViewerStateProvider> |
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.
Did we document in Vol-E app that ViewerStateProvider is required?
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.
Could we make an issue ticket in Vol-E to throw an error message when ViewerStateProvider is not provided?
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 a wrapper component that automatically includes the ViewerStateProvider would make this nice and clean for clients to integrate as a single component. It could just forward props through to the child.
| cellId: '', | ||
| imageUrl: '', | ||
| parentImageUrl: '', | ||
| visibleControls: { |
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.
This got changed from showControls to visibleControls, is that correct?
update vole-app
update some deps
make sure things build
remove the dtype to uint8 conversion
test demo.ipynb for functionality