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

Add ability to select columns for the last sample replicates #39

Merged
merged 5 commits into from
Jul 1, 2024

Conversation

kweav
Copy link
Collaborator

@kweav kweav commented Jun 25, 2024

⚠️ This is a stacked PR based on PR #38 ⚠️ and should complete the described necessities/additions for 3 parameters as described in PR #35

This parameter for selecting columns doesn't directly impact a filter, but does impact two graphs. So there are only changes to the R/plots-qc.R file and the inst/rmd/gimapQCTemplate.Rmd file. Specifically impacting the qc_variance_hist() and
qc_constructs_countzero_bar() functions. The changes to these functions include adding to the headers (param and description information), replacing anywhere numbers are hardcoded with the parameter, and sets the parameters to a default if they're null. Within the QC Template Rmd, changes include referencing the parameters in the function calls.

I can confirm that I knit the getting-started.Rmd successfully locally. The only oddity is that the heatmap is still showing up in the vignette rather than the QC report (as mentioned in PR #33)

Also need to setup something to check the identity of the target columns if they are not null as described in PR #38

@kweav kweav requested a review from cansavvy June 25, 2024 20:29
Copy link
Collaborator

@cansavvy cansavvy left a comment

Choose a reason for hiding this comment

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

A couple questions here but I think we're moving along!! 🎉

#' @description A short description...
#' @description This bar graph first uses the specified `filter_zerocount_target_col` columns to flag pgRNA constructs that have a raw count of 0 in any one of those columns/samples of interest.
#' Then, it looks at the specified columns for the final day/sample replicates (`filter_replicates_target_col`) to see for pgRNAs that were flagged by the filter, how many of those replicate samples had raw counts of zeros. And it produces a bar plot reporting on this.
#' Note, if you select samples/columns to check with the filter that don't have the replicate samples, this graph won't be informative. So you want there to be overlap between the columns for the two target_col parameters to have an informative graph
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great notes. This is excellent documentation!

R/plots-qc.R Outdated
@@ -113,18 +120,32 @@ qc_variance_hist <- function(gimap_dataset, wide_ar = 0.75){
#' @examples \dontrun{
#' gimap_dataset <- get_example_data("gimap")
#' qc_constructs_countzero_bar(gimap_dataset)
#'
#' #or if you want to select a specific column(s) for looking at where/which samples zero counts are present for
#' qc_constructs_countzero_bar(gimap_dataset, filter_zerocount_target_col = c(3:5))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You shouldn't need to have the c() here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I think you're right. I just hadn't gone through and removed them from all the PRs following the code review last week.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

pushed a change that simplifies the ones I found

R/plots-qc.R Outdated Show resolved Hide resolved
Co-authored-by: Candace Savonen <cansav09@gmail.com>
@kweav kweav changed the base branch from filter_qc_ki4 to main July 1, 2024 18:58
Copy link
Collaborator

@cansavvy cansavvy left a comment

Choose a reason for hiding this comment

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

LGTM!

@kweav kweav merged commit bc61e6b into main Jul 1, 2024
6 of 7 checks passed
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