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

Fixed "drop" argument errors preventing R check and build #61

Closed
wants to merge 29 commits into from

Conversation

drewjbeh
Copy link
Collaborator

@drewjbeh drewjbeh commented Feb 9, 2024

No description provided.

jwokaty and others added 26 commits April 25, 2023 11:17
* change from homemade gha scripts to biocthis gha workflow
* install tinytex v2 and packages
* include bioc devel test
Copy link
Collaborator

@c-mertes c-mertes left a comment

Choose a reason for hiding this comment

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

@drewjbeh thanks for the push on this front. Can you please go through my comments and then we can wrap it up.

R/AllGenerics.R Outdated
#' fds[1:10,2:3]
#' fds[,samples(fds) %in% c("sample1", "sample2")]
#' fds[1:10,by="ss"]
#' fds[1:10,2:3,,drop=FALSE]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this as a problem. As we do not want to ask the user to always set the drop argument. So we have to make sure it works without that for the end user. Within the package I'm fine with this. But outside on the user facing side we should not rely on this. to work properly.

R/AllGenerics.R Outdated
@@ -672,7 +673,7 @@ FRASER.results <- function(object, sampleIDs, fdrCutoff, zscoreCutoff,
ans <- lapply(seq_along(sampleChunks), function(idx){
message(date(), ": Process chunk: ", idx, " for: ", type)
sc <- sampleChunks[[idx]]
tmp_x <- object[,sc]
tmp_x <- object[,sc,,drop=FALSE]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you have the extra ,? They are not positional arguments so it should work without the extra missing argument or? This looks to me as wrong coding or some potential problematic code in the future.

R/AllGenerics.R Outdated
@@ -864,7 +865,7 @@ mapSeqlevels <- function(fds, style="UCSC", ...){
nonSplicedReads(fds) <- keepStandardChromosomes(nonSplicedReads(fds))
validObject(fds)
}
fds <- fds[as.vector(seqnames(fds)) %in% names(mappings)]
fds <- fds[as.vector(seqnames(fds)) %in% names(mappings),,,drop=FALSE]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above. Works for all the ,,, parts. can you please check this?

@@ -459,8 +459,8 @@ getSplitCountCacheFile <- function(sampleID, settings){
#' @export
countSplitReads <- function(sampleID, fds, NcpuPerSample=1, genome=NULL,
recount=FALSE, keepNonStandardChromosomes=TRUE,
bamfile=bamFile(fds[,sampleID]),
pairedend=pairedEnd(fds[,sampleID]),
bamfile=bamFile(fds[,sampleID,,drop=FALSE]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is kind of user facing code here. So not sure why we need it. I dont want to polute the code with things we do not need.

One Idea that could happen, is that fds is not a fraserdataset object but rather a summarizedexperiment object. Then the default with subsetting is drop=TRUE which could the underlying problem

#' junctions or splice sites
#' @param drop Currently not used
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you run devtools::document() i think this did not populate into the documentation.

@drewjbeh drewjbeh closed this Feb 10, 2024
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.

3 participants