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

begin EDA of sc time-lapse profiles #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MikeLippincott
Copy link
Member

This PR begins the single cell time-lapse EDA by generating UMAP embeddings and plotting the UMAP embedding space.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@MattsonCam
Copy link
Member

It looks like you're removing many other files in this repo, including some analysis. I think it would be useful to provide a change log for the software gardening decisions made in this pr. Also, it may be useful to answer some of these questions: WayScience/nf1_schwann_cell_morphology_signature#52 (comment)

Copy link
Member

@MattsonCam MattsonCam left a comment

Choose a reason for hiding this comment

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

I left some comments that I think should be considered before merging the pr. However, since you are responsible for deciding when to merge this pr, I will go ahead and approve

Copy link
Member

Choose a reason for hiding this comment

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

What is the NA label here? Consider changing this label

Copy link
Member

Choose a reason for hiding this comment

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

Is it common to use the means of the UMAP space in this way?

Copy link
Member

Choose a reason for hiding this comment

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

What does plotting the means of the UMAP components tell us?

Copy link
Member

Choose a reason for hiding this comment

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

Consider changing the x and y axis label to reflect the mean of the UMAP components (same for the other mean umap plots as well)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not seeing all of the time points here. If they're not present, consider removing them from the legend (same for the other mean umap plots as well)

Copy link
Member

Choose a reason for hiding this comment

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

It is difficult for me to distinguish between some of these time points. For example, between the 360 min and 330 min time points. Consider changing the color scheme to make the colors more distinguishable

Copy link
Member

Choose a reason for hiding this comment

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

Is this the same figure as the top row of the 0.EDA/figures/CP/umap_plot_time.png figure? If so, consider only keeping one copy to be more DRY.

Copy link
Member

Choose a reason for hiding this comment

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

Consider including a title describing details of the experiment, such as the compound used
(same comment for 0.EDA/figures/combined/umap_plot_time_part_of_doses.png)

Copy link
Member

Choose a reason for hiding this comment

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

Are these visualizations also present in 0.EDA/figures/combined/umap_plot_time.png If so, consider only keeping one copy to be more DRY.

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.

2 participants