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

Adding files from scib #4

Merged
merged 14 commits into from Jun 14, 2021
Merged

Adding files from scib #4

merged 14 commits into from Jun 14, 2021

Conversation

mumichae
Copy link
Collaborator

@mumichae mumichae commented Mar 5, 2021

Scripts added:

  • scripts/preprocessing.py
  • scripts/run_normalize.sh
  • scripts/sim_norm.py
  • R/visualization/*
  • notebooks/*

Corresponding PRs:

LuckyMD
LuckyMD previously approved these changes Mar 5, 2021
Copy link
Collaborator

@LuckyMD LuckyMD left a comment

Choose a reason for hiding this comment

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

ideally add the R/visualization stuff already now. But fine to do it in stages as well.

@mumichae mumichae requested a review from martaint March 6, 2021 04:44
@mumichae
Copy link
Collaborator Author

mumichae commented Mar 6, 2021

@martaint Is it safe to move all the R/visualization code here or are you still working on it?

@martaint
Copy link
Collaborator

martaint commented Mar 7, 2021 via email

@@ -0,0 +1 @@

Copy link
Member

Choose a reason for hiding this comment

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

Not sure where this blank file came from but can probably delete it.

Copy link
Member

@lazappi lazappi left a comment

Choose a reason for hiding this comment

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

Wow, that's a lot of notebooks! I assume somebody has already checked these are all still relevant?

  • Do we need the .ipynb_checkpoints directories?

Should we move the data files in the visualization/ directory to the top data/ directory? At least those that are for the paper not just examples for the plotting functions (@martaint might be able to help with that).

@mumichae
Copy link
Collaborator Author

mumichae commented Mar 8, 2021

Wow, that's a lot of notebooks! I assume somebody has already checked these are all still relevant?

We removed notebooks some time back (PR), so I think things should be OK. @LuckyMD @danielStrobl Could you double-check if there are any notebooks we don't need anymore?

* Do we need the `.ipynb_checkpoints` directories?

Good point, didn't realise that I was adding those. They should be removed and ignored now.

Should we move the data files in the visualization/ directory to the top data/ directory? At least those that are for the paper not just examples for the plotting functions (@martaint might be able to help with that).

I agree, that would be better. If we move the plot data, we just need to update the R scripts accordingly.

@LuckyMD
Copy link
Collaborator

LuckyMD commented Mar 10, 2021

Things I think might be removable:

  1. The empty file @lazappi highlighted
  2. Mouse atlas notebooks (@danielStrobl and @mumichae)
  3. notebooks/Visualization_ATAC.ipynb from @kridsadakorn
  4. notebooks/silhouette_graph.ipynb, notebooks/trajectory_plots and notebooks/visualization.ipynb from @danielStrobl
  5. visualization/atlas_heatmap.ipynb from @mumichae

Would love to hear if you think these can be removed or if this is important for reproducibility. If important, then maybe we need a further folder.

Copy link
Collaborator

@kridsadakorn kridsadakorn left a comment

Choose a reason for hiding this comment

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

notebooks/Vitualisation_ATAC.ipynb can be deleted because the plots were replaced by Daniel's version.

Naively, I tried to find a way to comment right under this file, but I couldn't figure out how to do.

PS. there is even a typo in the file name, poor me ...

@danielStrobl
Copy link
Contributor

The mouse atlas notebooks can be removed I think. I would keep the trajectory_plots and visualisation notebooks as the figures for the supplementary were generated with those. The silhouette graph notebook can be removed.

@danielStrobl
Copy link
Contributor

One thing I noticed is that the notebooks/visualisation.ipynb in master is not the most recent one, it's actually in the recompute_cluster_metrics branch

@LuckyMD
Copy link
Collaborator

LuckyMD commented Mar 31, 2021

Thanks for comments @danielStrobl. Could you say how you would name the folder (or in which existing folder) the notebooks to keep should be put? Just visualization?

@danielStrobl
Copy link
Contributor

Yes, sounds good

@lazappi
Copy link
Member

lazappi commented Apr 1, 2021

One thing I noticed is that the notebooks/visualisation.ipynb in master is not the most recent one, it's actually in the recompute_cluster_metrics branch

@danielStrobl Can you open a separate PR for the latest version (or add it here)? Or open an issue I guess, just don't want to forget about this.

P.S. I think you have a pending invite to join this repo, I can resend if you need.

@danielStrobl
Copy link
Contributor

Ok, I'll open a PR for that. That would be great if you could resend the invite.

@mumichae
Copy link
Collaborator Author

mumichae commented Apr 6, 2021

I agree that the heatmap and mouse atlas notebooks can be removed.

@mumichae
Copy link
Collaborator Author

mumichae commented Apr 6, 2021

I removed the heatmap plotting script (duplicate) and moved the preprocessing scripts to the Masterinternship_2019 repository.

@LuckyMD
Copy link
Collaborator

LuckyMD commented Apr 14, 2021

also, new analysis notebook updates from scIB.

@lazappi
Copy link
Member

lazappi commented Apr 15, 2021

Summarising things that have come up in comments:

Done

  • Resend invite to @danielStrobl
  • Remove .ipynb_checkpoints directories
  • Remove mouse atlas notebooks
  • Remove empty visualization/img/init file
  • Remove `notebooks/Vitualisation_ATAC.ipynb
  • Remove visualization/atlas_heatmap.ipynb
  • Remove notebooks/silhouette_graph.ipynb
  • Move visualisation notebooks to notebooks/visualization/

Todo

  • Move the visualisation data files to the data/ directory (this probably means other changes to scripts so maybe can be a later PR)`
  • Update notebooks/visualisation.ipynb to most recent version
  • Other notebook updates from scIB?
  • Add content to empty dataset READMEs

Anything that's incorrect or I missed?

Updated 2021-04-22

@lazappi
Copy link
Member

lazappi commented Apr 15, 2021

Also just noticed that each of the dataset notebook folders has an empty README. We should probably either add some info about the datasets or just delete them. @LuckyMD what do you think?

@LuckyMD
Copy link
Collaborator

LuckyMD commented Apr 15, 2021

Could you copy over some text from the dataset description in the paper to the README?

@lazappi
Copy link
Member

lazappi commented Apr 16, 2021

Could you copy over some text from the dataset description in the paper to the README?

That's definitely possible. Alternatively could describe what is in the notebooks (or both).

@LuckyMD
Copy link
Collaborator

LuckyMD commented Apr 16, 2021

Or maybe better: everyone could write a small blurb (3-4 sentences of what is in the dataset). I think that could even be from the SI sections.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@lazappi
Copy link
Member

lazappi commented Apr 22, 2021

Did some of the minor things, see updated checklist #4 (comment). Main things left to do are double checking notebooks are up to date (@danielStrobl can you help with that?) and deciding what to do with the READMEs.

@martaint
Copy link
Collaborator

martaint commented May 6, 2021

@lazappi I added a file in scib (theislab/scib#234) which should also be added here in the respective notebook folder. Could you take care of that? Don't want to mess with this PR :) thanks a lot!

lazappi
lazappi previously approved these changes Jun 14, 2021
@lazappi lazappi merged commit 7d91771 into main Jun 14, 2021
@lazappi lazappi deleted the cleanup branch June 14, 2021 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants