search

scverse / squidpy_notebooks

Tutorials for Squidpy
https://squidpy.readthedocs.io/en/stable/
MIT License
30 stars 18 forks source link

added tutorial notebook for vizgen mouse liver data #100

Closed cornhundred closed 2 years ago

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

cornhundred commented 2 years ago

Hi @giovp, we made a pull request with our Squidpy tutorial using Vizgen's Mouse Liver Map showcase data. Please let us know if you would like any changes or would like to discuss.

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

giovp commented on 2022-08-08T15:58:22Z ----------------------------------------------------------------

Line #1.    # Datasets can be obtained from the Vizgen Data release website (see above)

I can access the link but there are multiple datasets present in the website. Which files precisely should be downloaded?


_cornhundred commented on 2022-09-29T21:14:01Z_ ----------------------------------------------------------------

Thanks, I'll add the dataset name

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

giovp commented on 2022-08-08T15:58:23Z ----------------------------------------------------------------

Line #11.    meta_gene = meta_gene.loc[keep_genes]

in case you are interested, in squidpy dev (main branch) we have a working vizgen reader that also takes care of removing the blanks. https://squidpy.readthedocs.io/en/latest/api/squidpy.read.vizgen.html


_cornhundred commented on 2022-09-29T21:39:12Z_ ----------------------------------------------------------------

I wasn't able to install the latest version from pip (which I assume would have this read method) and I was getting the error 

ERROR: Could not find a version that satisfies the requirement networkx>=2.6.0 (from squidpy) (from versions: 0.34, 0.35, 0.35.1, 0.36, 0.37, 0.99, 1.0rc1, 1.0, 1.0.1, 1.1, 1.2rc1, 1.2, 1.3rc1, 1.3, 1.4rc1, 1.4, 1.5rc1, 1.5, 1.6rc1, 1.6, 1.7rc1, 1.7, 1.8rc1, 1.8, 1.8.1, 1.9rc1, 1.9, 1.9.1, 1.10rc2, 1.10, 1.11rc1, 1.11rc2, 1.11, 2.0, 2.1, 2.2rc1, 2.2, 2.3rc3, 2.3rc4, 2.3, 2.4rc1, 2.4rc2, 2.4, 2.5rc1, 2.5, 2.5.1)

ERROR: No matching distribution found for networkx>=2.6.0 (from squidpy)

WARNING: You are using pip version 20.1.1; however, version 21.3.1 is available.

You should consider upgrading via the '/Users/nickfernandez/anaconda3/bin/python -m pip install --upgrade pip' command.

so I will stick to this method for now if that is alright

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

giovp commented on 2022-08-08T15:58:24Z ----------------------------------------------------------------

Line #13.    # calculate barcode counts

I think it'd make sense to create an anndata right before this step and compute these metrics with https://scanpy.readthedocs.io/en/stable/generated/scanpy.pp.calculate_qc_metrics.html


_cornhundred commented on 2022-10-04T21:01:04Z_ ----------------------------------------------------------------

agreed, updating this

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

giovp commented on 2022-08-08T15:58:25Z ----------------------------------------------------------------

Line #17.    # filter out cells with low expression

these steps could also be performed with

  • https://scanpy.readthedocs.io/en/stable/generated/scanpy.pp.filter_cells.html
  • https://scanpy.readthedocs.io/en/stable/generated/scanpy.pp.filter_genes.html

you can see in the current notebooks for vizgen how we do it (maybe incorrect, I'd be happy to change it if you think so)

https://squidpy.readthedocs.io/en/latest/external_tutorials/tutorial_vizgen.html


_cornhundred commented on 2022-09-30T17:37:53Z_ ----------------------------------------------------------------

looks good, updating this

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

giovp commented on 2022-08-08T15:58:25Z ----------------------------------------------------------------

Line #2.    mat_spatial = deepcopy(adata.obs[['center_x', 'center_y']])

what about :

adata.obsm['spatial'] = adata.obs[['center_x', 'center_y']].copy()


_cornhundred commented on 2022-10-04T20:56:42Z_ ----------------------------------------------------------------

looks good

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

giovp commented on 2022-08-08T15:58:26Z ----------------------------------------------------------------

Line #1.    sc.set_figure_params(figsize=(10,10))

figure_params set already above


_cornhundred commented on 2022-10-04T20:56:52Z_ ----------------------------------------------------------------

thanks

review-notebook-app[bot] commented 2 years ago

View / edit / reply to this conversation on ReviewNB

giovp commented on 2022-08-08T15:58:27Z ----------------------------------------------------------------

in the outputs there is always this message, I wonder if it's possible to remove it?

<IPython.core.display.Javascript object>


cornhundred commented on 2022-10-04T20:54:03Z ----------------------------------------------------------------

yes, this is an artifact from running it on Google Colab

giovp commented 2 years ago

hi @cornhundred !

Thanks a lot for this very nice tutorial! I left few comments mostly on the pre-processing/qc.

The lint job is failing because of the spell checker. Basically, to avoid typos and ensure that we refer to concepts always with the same word, we have a spell checker list of words that should be added in case they are not recognized. It can be found here: https://github.com/scverse/squidpy_notebooks/blob/55ce4895afe15227d6eda6213f80d4e4ce1009c9/docs/source/spelling_wordlist.txt

you can see from the linting error that while some are real typos (e.g. investiagate ) other are just new words (which should maybe spelled always the same way?). For instance, gene names ( e.g.Vwfetc.) could simply be added to the wordlist. One big issues seems to be the Peri-central (you could just addPeri`) but that should resolve most of the errors.

Let me know if it's clear and whether you think the comments make sense! Happy to discuss further.

Thanks again for this great contribution!

cornhundred commented 2 years ago

Yes, I'll add a direct link to the sign up page

View entire conversation on ReviewNB

cornhundred commented 2 years ago

I wasn't able to install the latest version from pip (which I assume would have this read method) and I was getting the error 

ERROR: Could not find a version that satisfies the requirement networkx>=2.6.0 (from squidpy) (from versions: 0.34, 0.35, 0.35.1, 0.36, 0.37, 0.99, 1.0rc1, 1.0, 1.0.1, 1.1, 1.2rc1, 1.2, 1.3rc1, 1.3, 1.4rc1, 1.4, 1.5rc1, 1.5, 1.6rc1, 1.6, 1.7rc1, 1.7, 1.8rc1, 1.8, 1.8.1, 1.9rc1, 1.9, 1.9.1, 1.10rc2, 1.10, 1.11rc1, 1.11rc2, 1.11, 2.0, 2.1, 2.2rc1, 2.2, 2.3rc3, 2.3rc4, 2.3, 2.4rc1, 2.4rc2, 2.4, 2.5rc1, 2.5, 2.5.1)

ERROR: No matching distribution found for networkx>=2.6.0 (from squidpy)

WARNING: You are using pip version 20.1.1; however, version 21.3.1 is available.

You should consider upgrading via the '/Users/nickfernandez/anaconda3/bin/python -m pip install --upgrade pip' command.

so I will stick to this method for now if that is alright

View entire conversation on ReviewNB

cornhundred commented 2 years ago

looks good, updating this

View entire conversation on ReviewNB

cornhundred commented 2 years ago

yes, this is an artifact from running it on Google Colab

View entire conversation on ReviewNB

cornhundred commented 2 years ago

looks good

View entire conversation on ReviewNB

cornhundred commented 2 years ago

thanks

View entire conversation on ReviewNB

cornhundred commented 2 years ago

agreed, updating this

View entire conversation on ReviewNB

cornhundred commented 2 years ago

Hi @giovp, thanks for the feedback. I just committed the update, let us know what you think.

FYI - I also switched from Google Colab to my local MacBook to run the notebook so some of the Leiden cluster numbers are slightly different now, but the conclusions remain the same.

cornhundred commented 2 years ago

Hi @giovp, just checking if you had a chance to review the changes. Please let us know if you need anything or want anything modified for the tutorial notebook.

giovp commented 2 years ago

hi @cornhundred ,

looks great, I'd be happy to merge it today/this weekend and cut new release Monday morning! However, it seems that CI is not running on this branch (and also not in your fork) and that I can't checkout the PR locally 

From https://github.com/scverse/squidpy_notebooks
 ! [rejected]        refs/pull/100/head -> tutorial_vizgen_mouse_liver  (non-fast-forward)

is it possibly some settings in your organization that prevent to do that?

giovp commented 2 years ago

I need to check it out locally as I'd have to add specific entry in the HTML code for thumbnail rendering.

giovp commented 2 years ago

also one last thing, would you like to add yourself explicitly as an author of this notebook? If so, feel free to do it in the header!

cornhundred commented 2 years ago

Thanks @giovp! Sure I can add authorship,do you mean in the markdown header?

giovp commented 2 years ago

Thanks @giovp! Sure I can add authorship,do you mean in the markdown header?

yes! will do

cornhundred commented 2 years ago

Thanks for all of your help, we're so glad we were able to get a tutorial set up for Squidpy with our liver showcase data 😀 Please let me know if you need anything else from us.

giovp commented 2 years ago

@cornhundred actually I do, see message above the CI is not running, neither here nor in your fork, and I can't pull locally the notebook, which I have to do in order to add the thumbnail link for the doc. Unfortunately, I don't know how to proceed, it might be a setting in your fork (maybe you'd have to enable CI or add me as collaborator?)

cornhundred commented 2 years ago

Thanks for letting us know the issue. @psoares from our group might be able to help, he previously mentioned we might be missing some token on our end in order to run the CI. I also added you as an outside collaborator (triage) since it is a public fork of your project. Let me know if adding you as a collaborator helped, otherwise we can troubleshoot on our side.

psoares commented 2 years ago

It seems to run, but there were linting failures, right? See https://github.com/scverse/squidpy_notebooks/actions/runs/3256350851

cornhundred commented 2 years ago

thanks @psoares, I tried to address the spelling linting errors from that link. We'll see if I missed anything on the next check.

giovp commented 2 years ago

hi @cornhundred @psoares ,

thank you so much, indeed there were many spellcheck. There seems to be also an inconsistent title (probably due to wrong use of markdown formatting, see e.g. this StackOverflow answer ).

Couple more things while I tried to runt he notebooks (see the nb preview from this PR https://github.com/scverse/squidpy_notebooks/pull/103)

since the scanpy equivalent will be deprecated soon.

Let me know what you think!

cornhundred commented 2 years ago

Working on the changes and I'll push a new commit soon

cornhundred commented 2 years ago

@giovp, for this latest commit I switched to the latest version of Squidpy (version 1.2.2), used the sq.read.vizgen method, installed openpyxl, modified the headers to hopefully address the with titles we saw, and switched to sq.pl.spatial_scatter method. I had to make some small modifications to the markdown to reflect the slightly shifted Leiden clusters after the version update. Thanks again for the suggestions and let me know if you would like any other modifications.

One small thing I noticed was that the size of the spatial plots in the sq.pl.spatial_scatter method seemed to be effected by the legend size (e.g., number of and length of the names).

giovp commented 2 years ago

thanks a lot for this @cornhundred , for some reasons I still get this error

❯ git push -f origin HEAD:refs/pull/100/head
Enumerating objects: 82, done.
Counting objects: 100% (79/79), done.
Delta compression using up to 4 threads
Compressing objects: 100% (64/64), done.
Writing objects: 100% (64/64), 89.50 MiB | 1.16 MiB/s, done.
Total 64 (delta 49), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (49/49), completed with 8 local objects.
To https://github.com/scverse/squidpy_notebooks.git
 ! [remote rejected] HEAD -> refs/pull/100/head (deny updating a hidden ref)
error: failed to push some refs to 'https://github.com/scverse/squidpy_notebooks.git'

might be something from my side, but I can only push to a new branch and to this one.

giovp commented 2 years ago

merged in #106 this can be closed, thank you again for this contribution and pushing it to the finish line! Will release soon.

  • © Githubissues.
  • Githubissues is a development platform for aggregating issues.