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

Add Xenium tutorial using spatialdata #125

Merged
merged 7 commits into from
Sep 2, 2024
Merged

Conversation

LLehner
Copy link
Member

@LLehner LLehner commented May 26, 2024

An updated Xenium tutorial for analysis using Squidpy, but instead of only working with anndata, spatialdata is used.

@LucaMarconato please let me know if things should be added. So far I just reproduced the original Xenium tutorial with spatialdata.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@LLehner LLehner added the documentation Improvements or additions to documentation label May 26, 2024
Copy link
Member

@LucaMarconato LucaMarconato left a comment

Choose a reason for hiding this comment

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

Thanks for the notebooks, asking for some (minor) changes:

  • please write adata = sdata.tables['table'] and then use adata. It is less verbose and it has the same effect because adata is then a reference to sdata.tables['table']
  • please explain that:
  • please keep the last plot, but also remake it using spatialdata-plot, to show the user that they have also this possibility.
  • Also I would add (commented), the code to fire up napari (from napari import Interactive followed by Interactive(sdata))

@LucaMarconato
Copy link
Member

LucaMarconato commented Jun 16, 2024

Extra thing: @giovp after the code that you wrote in the latest hackathon on graph support for squidpy, is there anything additional to add? If not needed, after the (quick) changes above, we can merge 😊

@giovp
Copy link
Member

giovp commented Jul 8, 2024

Extra thing: @giovp after the code that you wrote in the latest hackathon on graph support for squidpy, is there anything additional to add? If not needed, after the (quick) changes above, we can merge 😊

yes I think we could be using the new gr.spatial_neighbors that support directly spatialdata, but for that I think we'd first need to merge the PR/do release

@LucaMarconato
Copy link
Member

Thanks for the fixes @LLehner! I think we are good to merge @giovp

@giovp
Copy link
Member

giovp commented Aug 23, 2024

super nice! I just reviewed it and there are few aesthetics that should be modified, for the rest it looks great! I would merge after the points in the review have been addressed

Copy link

review-notebook-app bot commented Aug 23, 2024

View / edit / reply to this conversation on ReviewNB

giovp commented on 2024-08-23T15:38:30Z
----------------------------------------------------------------

Line above I would remove, it appears elsewhere in the nb


Copy link

review-notebook-app bot commented Aug 23, 2024

View / edit / reply to this conversation on ReviewNB

giovp commented on 2024-08-23T15:38:30Z
----------------------------------------------------------------

also here and above


LLehner commented on 2024-08-27T08:07:22Z
----------------------------------------------------------------

Shouldn't some titles remain so people find certain sections more easily?

giovp commented on 2024-08-30T21:10:37Z
----------------------------------------------------------------

yeah but it doesn't look like they are rendered as title right? or you mean that the rendering here is incorrect with respect to how it will be render for in RTD? The lines are not about not having titles, but having titles rendered correctly, here I think the problem is that both --- and # are used. Does it make sense?

Copy link

review-notebook-app bot commented Aug 23, 2024

View / edit / reply to this conversation on ReviewNB

giovp commented on 2024-08-23T15:38:31Z
----------------------------------------------------------------

would be nice maybe a screenshot to add here of the napari plugin open?


Copy link
Member Author

LLehner commented Aug 27, 2024

Shouldn't some titles remain so people find certain sections more easily?


View entire conversation on ReviewNB

Copy link
Member

giovp commented Aug 30, 2024

yeah but it doesn't look like they are rendered as title right? or you mean that the rendering here is incorrect with respect to how it will be render for in RTD? The lines are not about not having titles, but having titles rendered correctly, here I think the problem is that both --- and # are used. Does it make sense?


View entire conversation on ReviewNB

@giovp
Copy link
Member

giovp commented Aug 30, 2024

see the diffs here regarding the misunderstanding between lines and titles
image
image

or

image

please re-add the titles, but make sure that they are rendered correctly, before there was the title AND the --- which were extra

@giovp
Copy link
Member

giovp commented Aug 30, 2024

e.g. see what I mean here, the title is in both, and the text should stay, but there is the line --- which is unclear why is it there

image

@giovp
Copy link
Member

giovp commented Aug 30, 2024

in the current read the docs latest they are not present

image

@giovp giovp merged commit 79257eb into main Sep 2, 2024
1 check passed
@giovp giovp deleted the xenium_spatialdata_tutorial branch September 2, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants