-
Notifications
You must be signed in to change notification settings - Fork 5
Segmentation result retrieval and plotting in Jupyter notebook #194
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
base: main
Are you sure you want to change the base?
Conversation
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.
Nice work! This PR has been previously tested and it works as expected. I left some minor comments that should be addressed prior merging.
" metadata_field - the field to extract\n", | ||
"\"\"\"\n", | ||
"tiled_base_uri = \"http://127.0.0.1:8000\"\n", | ||
"additional_string_tiled_uri = \"writable\"\n", |
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 we need to define an additional_string_tiled_uri
or is it sufficient to have a single seg_tiled_uri
(similarly to .env.example)? In line with this comment, should we add the option to read the SEG_TILED_API_KEY
?
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.
We have the additional_string_tiled_uri
for the notebook user to change the string as needed. But if there is only one uri being accessed, then we could make it a single string, instead of having a base uri and additional string.
examples/tiled_ipywidget.ipynb
Outdated
"metadata": {}, | ||
"outputs": [], | ||
"source": [ | ||
"from tiled.client import from_uri" |
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.
Should we have a single cell with imports?
" dropdown_list = []\n", | ||
"\n", | ||
" for experiment in client:\n", | ||
" if \"seg_result\" in client[experiment]:\n", |
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 works quite nicely. I am mostly wondering if we can use the query built-in capabilities in Tiled (e.g. tiled-query) since the operation as-is may become more costly as the number of segmented results increases.
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 wasn't aware of the query capabilities. Thank you for letting me know!
The current implementation can get costly, but that was the way we are iterating through scans in a different project, and hence, I did the same here. Did not know of another way. Changes will be made to make this more efficient.
"\n", | ||
"\n", | ||
"def tiled_refresh_button_click(button):\n", | ||
" tiled_widget_refresh_callback(tiled_file_widget)\n", |
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 have not worked with IPyWidgets before - is wrapping the function like this needed for the callback?
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.
The documentation I came across seemed to do it this way. I tried to avoid this way, but there was no way around it. I can look further into other examples possibly and get back on this.
"selected_scan_uri = tiled_file_widget.value\n", | ||
"\n", | ||
"fig = plt.figure(layout=\"constrained\")\n", | ||
"plot_seg_mask(selected_scan_uri, 0, fig.gca())\n", |
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 also works nicely. A point of improvement could be to have a function that creates the overall plot and that automatically detects how many subplots are needed - depending on how many frames we have, so that the function is called once only for the set of segmentation results that were selected.
This PR adds an example notebook that shows how segmentation results saved in Tiled can be retrieved and plotted in a Jupyter notebook.
Segmentation results can be selected from a
ipywidget
dropdown showing the values for a selected meta data key per result. A suitable meta data key is thejob_name
of a segmentation job, that is added to segmentation results in #193 and mlexchange/mlex_dlsia_segmentation_prototype#34.Bulk of the work by @rajasriramoju.