-
Notifications
You must be signed in to change notification settings - Fork 26
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
Visualization example #171
Conversation
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.
Adding on the dream team to give more detailed feedback. In general, the notebook needs more detailed comments. Take a look at https://github.com/angelolab/ark-analysis/blob/master/templates/Deepcell_Postprocessing.ipynb for an example. The idea is that this should be fairly self-explanatory
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.
Great start! A few comments about aesthetics and good coding practices.
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.
Not much to add to Alex's comments
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.
Same as what Adam and Alex mentioned. If you're submitting a PR, it means you think this is ready to be included in the main version of the github for everyone to see. So try and do any necessary cleanup beforehand, like error messages, before that happens.
Also, the point of these notebooks is both for people to be able to run them, but also as an example of what our code can do. So having the plots already generated is helpful for people who are just browsing through.
Let me know if you guys think this is useful: https://app.reviewnb.com/angelolab/ark-analysis/pull/171/files/ |
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.
Looking good, need to remove the ModuleNotFoundError from the notebook however. @wang0324 if you're having other people run the notebook for you, ask them to make sure all errors are removed before sending it over for you to push. Or, you can ask them to push to this branch instead after they've ironed all the errors out.
View / edit / reply to this conversation on ReviewNB ngreenwald commented on 2020-08-25T06:18:40Z We don't need to repeat these long lists of markers again |
View / edit / reply to this conversation on ReviewNB ngreenwald commented on 2020-08-25T06:18:40Z Same thing here, I don't think we need to show all 6 combinations of plot type and overlay type. Maybe just include one UMAP, one tSNE, and have one of them be patient and the other by cell lineage. I think all of these should be lineage, not cell type, like I mentioned for your lab meeting. It will make them more interpretable |
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.
See comments from reviewnb
definitely a lot more useful |
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.
Looks like you added all of the files that got generated. We don't want to have those in the repo, just the example script
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.
Great job!
* started jupyter notebook * Updated and Added Function Calls * Added data for 4 patients * added more example calls * switched to new name * documentation * init commit * added comments * tested * ran test * Fixed UMAP import * generated graphs * ran example spatial w/ error * reran it * no more errors * fixed data * Fixed x-axis labels * New Graphs working * Added save directory * Fixed spelling error * added heading * generated graphs * Fixed typo * Fixed File Directory * coverage * removed generated files Co-authored-by: Neeraj Rattehalli <neeraj.rattehalli@gmail.com> Co-authored-by: Kevin Chu <kevinlc221@gmail.com> Co-authored-by: William Xu <williamxu132@gmail.com>
* started jupyter notebook * Updated and Added Function Calls * Added data for 4 patients * added more example calls * switched to new name * documentation * init commit * added comments * tested * ran test * Fixed UMAP import * generated graphs * ran example spatial w/ error * reran it * no more errors * fixed data * Fixed x-axis labels * New Graphs working * Added save directory * Fixed spelling error * added heading * generated graphs * Fixed typo * Fixed File Directory * coverage * removed generated files Co-authored-by: Neeraj Rattehalli <neeraj.rattehalli@gmail.com> Co-authored-by: Kevin Chu <kevinlc221@gmail.com> Co-authored-by: William Xu <williamxu132@gmail.com>
Added jupyter notebook showcasing examples of plotting and dimensionality reduction functions