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

Fix bugs in visualize segmentation #357

Merged
merged 16 commits into from
Dec 3, 2020
Merged

Fix bugs in visualize segmentation #357

merged 16 commits into from
Dec 3, 2020

Conversation

alex-l-kong
Copy link
Contributor

@alex-l-kong alex-l-kong commented Dec 1, 2020

What is the purpose of this PR?

We've received a lot of bugs that have resulted from merging #193 in. This PR encompasses the following 3 issues:

How did you implement your changes

It seems that the bugs aren't completely related to visualize_segmentation but rather on what's being passed in. data_xr instead of data_xr_summed, for example, was incorrectly passed in, which likely caused @awedwards issue.

chan_list verification logic was a little buggy, so that was improved as well.

@alex-l-kong alex-l-kong self-assigned this Dec 1, 2020
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@alex-l-kong
Copy link
Contributor Author

alex-l-kong commented Dec 1, 2020

So this method works to fix visualize_segmentation in the notebook for now (which should address at least @awedwards issue), but the conflicts between channels and compartments is definitely a minor headache. Would be helpful to discuss alternatives for the next code review.

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks good. Can you update the description with which of the issues this is the closing?

The thing I don't like about visualize_segmentation is that there are two separate things people will want to do.

  1. Cycle through different example images in the notebook, seeing how the overlays look
  2. Save the segmentation output for all FOVs in their dataset.

Right now, they're combined together into the same function. This is partly what lead to the show=true errors. What do you think the best way to split these up would be?

@alex-l-kong
Copy link
Contributor Author

Looks good. Can you update the description with which of the issues this is the closing?

The thing I don't like about visualize_segmentation is that there are two separate things people will want to do.

  1. Cycle through different example images in the notebook, seeing how the overlays look
  2. Save the segmentation output for all FOVs in their dataset.

Right now, they're combined together into the same function. This is partly what lead to the show=true errors. What do you think the best way to split these up would be?

@ngreenwald I think the best way to do this is to have plot_overlay only have imshow calls and visualize_segmentation only have imsave calls. Right now, it's also possible for plot_overlay to save an image and I agree, it's really confusing. We can accomplish this by simply returning the result of what would normally be saved by plot_overlay, then allow visualize_segmentation to save it for us.

This is great for making the functionality clearer, but in terms of the issues people have been bringing up, I don't think it's actually show=True that's causing the problem anymore, but rather other errors (ex. data_xr and not data_xr_summed, quite possibly user-specific data issues and filesystem errors, etc.) that just happen to be seen by this particular call to plot_overlay first. I'm doing more investigating into these to see what the root problems are, but I do believe something is getting propagated into the call to plot_overlay with show=True (especially for #354).

@ngreenwald
Copy link
Member

The get_topmost_subplot error happens when you create two different plots in the same plotting window. This is happening because of the for loop, where each FOV is getting imshowed. The error goes away if only a single FOV is generated.

Okay great, I agree. I think as a further simplification, we don't even need visualize_segmentation to call plot_overlay for generating the RGB overlay. We can just have a separate cell that directly calls plot_overlay to generate the overlay people care about (the channel overlay).

Once they're done with that, then we can have them call visualize_segmentation (which we'll call something else). That function will loop through, and save the output files for all of the fovs in the xarray

@alex-l-kong
Copy link
Contributor Author

@ngreenwald a few questions:

  1. The call to plot_overlay will display only the result for a single fov at a time, as opposed to multiple at a time?
  2. With this new method, looks like we're only going to be saving _segmentation_borders.tiff and _segmentation_labels.tiff files?
  3. Have we already tried using plt.imshow instead of io.imshow? I think plt.imshow could allow us to display multiple channel overlays at once, although that would be a problem if the user passed in 100 fovs.

@ngreenwald
Copy link
Member

I think we should move all of the stuff that's in plot_overlay which doesn't involve plotting a multi-color overlay and move it to the visualize_segmentation function, which we can rename to save_segmentation_output or something like that.

Then there will be one cell where the user calls plot_overlay directly, with a single FOV specified, to look at the output. And a separate cell which loops over all of the data and produces the borders.

@ngreenwald
Copy link
Member

Even better, we should modifiy plot_overlay to be create_overlay. Then it can return the image to displayed in the notebook. save_segmentation_output can then also call create_overlay, but without displaying the image each time.

Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few last remaining skeletons to remove from the plot function that can be moved into the segmentation_labels function, so that all the plot function is responsible for is taking an image and creating an RGB overlay

ark/utils/plot_utils.py Outdated Show resolved Hide resolved
ark/utils/plot_utils.py Outdated Show resolved Hide resolved
ark/utils/plot_utils.py Outdated Show resolved Hide resolved
ark/utils/segmentation_utils.py Outdated Show resolved Hide resolved
ark/utils/segmentation_utils.py Outdated Show resolved Hide resolved
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Nice, almost there

ark/utils/plot_utils.py Outdated Show resolved Hide resolved
ark/utils/plot_utils.py Show resolved Hide resolved
ark/utils/plot_utils.py Outdated Show resolved Hide resolved
ark/utils/segmentation_utils.py Show resolved Hide resolved
ark/utils/segmentation_utils.py Outdated Show resolved Hide resolved
@alex-l-kong
Copy link
Contributor Author

@ngreenwald OK, the new channel structure has been added, ready for review.

@alex-l-kong alex-l-kong requested review from ngreenwald and removed request for ngreenwald December 2, 2020 22:56
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Close, just need to update the colors of the input data

ark/utils/plot_utils.py Outdated Show resolved Hide resolved
ark/utils/segmentation_utils_test.py Outdated Show resolved Hide resolved
Copy link
Member

@ngreenwald ngreenwald left a comment

Choose a reason for hiding this comment

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

Nice stuff

raise ValueError("max 3 channels of overlay supported, got {}".
format(plotting_tif.shape))

# set first n channels (in reverse order) of formatted_tif to plotting_tif
Copy link
Member

Choose a reason for hiding this comment

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

clever!

@ngreenwald ngreenwald merged commit dc2c7b8 into master Dec 3, 2020
@ngreenwald ngreenwald deleted the plot_overlay_fix branch December 3, 2020 19:26
alex-l-kong added a commit that referenced this pull request Jan 14, 2021
* Fix bug in Segment_Image_Data: change data_xr to data_xr_summed

* Handle cases for both compartments and channels

* Make visualize_segmentation easier to understand

* Change compartments to channels and add comment allowing user to set fovs_to_display to None

* Logic and testing added for new plot overlay workflow

* Last commit before nuking previous implementation

* Nuke previous implementation

* Documentation fix

* Address code review

* Fix call in notebook

* Fix documentation for extract_delimited_names

* Address code review comments

* Assign to RGB channels in BGR order

* Add notebook visualization

* Address (hopefully) final review comments
y2kbugger pushed a commit that referenced this pull request Jul 29, 2021
* Fix bug in Segment_Image_Data: change data_xr to data_xr_summed

* Handle cases for both compartments and channels

* Make visualize_segmentation easier to understand

* Change compartments to channels and add comment allowing user to set fovs_to_display to None

* Logic and testing added for new plot overlay workflow

* Last commit before nuking previous implementation

* Nuke previous implementation

* Documentation fix

* Address code review

* Fix call in notebook

* Fix documentation for extract_delimited_names

* Address code review comments

* Assign to RGB channels in BGR order

* Add notebook visualization

* Address (hopefully) final review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants