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

Fix bugs for dealing with processed objects with only 1 cell #755

Merged
merged 7 commits into from
Apr 25, 2024

Conversation

allyhawkins
Copy link
Member

While processing data for the Portal, I found an object that only has 1 cell in the processed object so it tested out some of the code we added to handle that scenario. There were a few issues that I had to fix to get this to work and then I also made a design change that we will need to decide on if it's the right move or not.

  • If any object only has 1 cell then it doesn't have PCA or clusters. We have a check for PCA/UMAP when generating the reports, but not for clusters. Here I added a has_clusters variable to make sure no plots that use clusters are created.
  • There was also a small error in the add_celltypes_to_sce.R where we were still attempting to add in cell type annotations if the predictions file was empty. So I adjusted the logic there to account for the missing file properly.

The other thing I did was choose to not create the supplemental report if there's only 1 cell in the object. The report was just some text with one table showing the assignment of that single cell in SingleR, but no other plots. This doesn't seem super informative to me, and we would still have the table in the main QC report. What do others think of this choice? This should not affect very many samples, but we would want to make sure we check how this affects zipping up the files for the Portal.

@davidsmejia Would you guys have to make changes when zipping up the files for the Portal if a sample was missing the celltype-report.html file? I assume you have checks to make sure the files are all there, so would we be able to make the existence of this file optional?

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

LGTM. I just had a number of linting places where I changed & -> && and | -> ||. None of them matter in this case, but good practice.

templates/qc_report/celltypes_qc.rmd Outdated Show resolved Hide resolved
templates/qc_report/celltypes_qc.rmd Outdated Show resolved Hide resolved
templates/qc_report/celltypes_qc.rmd Outdated Show resolved Hide resolved
templates/qc_report/celltypes_supplemental_report.rmd Outdated Show resolved Hide resolved
templates/qc_report/celltypes_supplemental_report.rmd Outdated Show resolved Hide resolved
bin/add_celltypes_to_sce.R Outdated Show resolved Hide resolved
Co-authored-by: Joshua Shapiro <josh.shapiro@ccdatalab.org>
@allyhawkins
Copy link
Member Author

@jashapiro are we okay with the decision to not create a supplemental report? I just want to be sure before I merge this and then update the current release.

@jashapiro
Copy link
Member

@jashapiro are we okay with the decision to not create a supplemental report? I just want to be sure before I merge this and then update the current release.

Yes, I am fine with that decision!

@allyhawkins
Copy link
Member Author

Yes, I am fine with that decision!

Can you approve please? 😄

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

Sorry, thought I did!

@allyhawkins allyhawkins merged commit d5ff33b into main Apr 25, 2024
4 checks passed
@allyhawkins allyhawkins deleted the allyhawkins/no-clusters branch April 25, 2024 18:41
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