-
Notifications
You must be signed in to change notification settings - Fork 922
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 support for multi-modal segmentation #8605
Conversation
@AustinHartman would be great to also get a review on this PR. Thanks! |
@dcollins15 @rsatija ping on this -- what can we do to move this forward? |
any updates on this? |
Sincere apologies for the delay. We've been slow to respond to PRs but are going to prioritize this going forward. We'll get back to you soon but to confirm, @pmarks you are signed off on this request? |
@rsatija thanks for taking a look! Yes, should be ready to go. |
Hi @pmarks , sounds great. We're reviewing and just wondering if you could point us to a public dataset for testing, thanks! |
Cool! This dataset includes multi-modal segmentation: |
Thanks all! Just resolved a conflict that popped up on this branch. Please let me know if there's anything else I can do to help get this reviewed! |
Hi @rsatija @dcollins15, sorry to keep pestering you on this but is there any progress on getting this merged and into a CRAN release? We have quite a few reports of users being blocked in their analyses by the change to Thanks! |
Thanks! Agreed that we need to move forward with this. To me, everythign looks good, and I see that you are up-to-date with our develop branch @dcollins15 if you or the team see any issues here please let @jsicherman know, and if not, we'll go ahead and merge this in a week from today. |
Seems |
You can use this repo https://github.com/10XGenomics/seurat/tree/develop for now |
@ScienceComputing I just confirmed that |
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.
Apologies @jsicherman and @pmarks, I was under the impression that some feedback had been communicated outside of this thread, it wasn't my intent to leave you hanging!
Aside from one last copy-paste error, things seem to be working as expected with the Human Lung Cancer and Human Pancreas datasets. Unfortunately, I'm still unable to parse the older Mouse Brain Coronal Subset data (see comments below).
Once everything is working, there are a few documentation-related tasks to handle before this can be safely merged:
stars
and (maybe)gmp
should be added to theDESCRIPTION
underSuggests
(i.e., listed as a suggested dependency)- The
@importFrom utils unzip
tag needs to be added to the docstring forReadXenium
- The
NAMESPACE
andman/*.Rd
files need to be updated — this can be done usingroxygen2::roxygenise
ordevtools::document
- An entry should be added to the
NEWS.md
file describing the changes introduced in this PR. The more detail provided in the changelog, the better for users
After those are taken care of, the final step is to bump the devel version and date in the DESCRIPTION
file. Since another change is just about ready to be merged, I expect this will land as v5.1.0.9006
🚀
Thanks for the thorough review @dcollins15! Will try to put up some fixes on Monday. Should be able to remove the softdepends on |
Thank you so much for the updates. I tried the new dev version of Seurat, but the Xenium 5K data still cannot be loaded normally. Any insights on this issue? Error: file not found
|
seems to be related ->
|
Yes, I do have the newly released GDAL. What’s the next step I should take to successfully load the data? Highly appreciate your help and time.
|
@ScienceComputing try re-installing ( @dcollins15 let me know if you want me to tackle these on this PR or if that's something you'd rather have your team manage. Changelog below.
|
Thank you so much for the updates! I've been really looking forward to this to be able to analyze my Xenium data. (multi tissue Mouse panel + 100 custom genes) Error in CreateAssayObject():
|
@YfCem Were any edits made to the contents of
You should see cell IDs. This error would suggest to me that, for some reason, that file is empty? Though admittedly I haven't dug around in the code to load the MEX outputs too much. |
@jsicherman Thanks for getting back to me so quick! I gave your recommendation a try and this is what I got:
I'm interpreting this as the file is not empty? Appreciate all the help! |
Probably it is now solved with the latest commits. |
Thanks for getting these updates pushed up so quickly, @jsicherman! I'll have another round of comments ready for you tomorrow 👌 To answer your question — ideally, the NAMESPACE, man/*.Rd, and NEWS.md updates should be made before this PR is merged. I'd be happy to take care of the final updates once the other changes are ready, but I don't think I have write access to the 10XGenomics/seruat:develop branch. I'll also take a stab at refining the changelog you provided to ensure consistency — we appreciate you getting it started 🙏 In general, things are looking sweet! Excited to get this functionality merged and released 🤓 |
Thank you so much for your wonderful updates! It works! |
Yes, the Xenium 5K data works well with your team's latest commits. Thank you sooo much for your amazing help & suggestion. Absolutely, would like to give it try! |
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 🚀 thanks for all your patience @jsicherman
I've got one small error-handling nit below — it's non-blocking and easy for us to address in quick follow-up PR, so I'll let you decide if it's worth adjusting now 😄
Just a heads-up: none of the datasets I've been using appear to have the segmentation_method
column in either of the cells.csv.gz"
or cells.parquet
files. It's entirely possible I might have pulled down the wrong versions...
The last piece of housekeeping is to bump the version and date in the DESCRIPTION
file and then I'll merge this in as v5.1.0.9006
🥳
We can directly visualize cells which were segmented according to each method. | ||
|
||
```{r} | ||
ImageDimPlot(xenium.obj, fov = "fov", dark.background = F, group.by = "segmentation_method", cols = c('#ffabc3', '#a9a900', '#a9ceff')) |
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.
Non-blocking but this line is throwing an error because of the missing meta.data column. We need to make some other updates to this vignette before our next release so we can make any required adjustments then.
Thanks @dcollins15! Appreciate the discussion on the PR 🚀 ! |
This adds the ability to read
segmentation_method
from thecells.zarr.zip
for Xenium + multi-modal segmentation datasets. Zarr reading does not seem to be very straightforward with R, though, so some softdepends are needed. An example of this is shown, as well as some demonstrations on how to load other XOA outputs, in a small addition to the spatial vignette.I also used this opportunity to clean up Xenium reading a bit, and since we will be moving from .csv.gz outputs to exclusively .parquet outputs, added support for that.