Conversation
…es for those functions that currently have it hardcoded, giving more control over where the output files are saved. Especially useful for multi-dataset runs in a pipeline.
…and QC functions to gives more control to user for high quality publication figures
Reviewer's GuideThe PR adds configurable, consistently vectorized PDF plot output and propagates custom filename stems through the pipeline, while also introducing dataset-level regulatory-mode state, a combined single-data-frame output, and broad validation and result-handling changes that require review beyond the stated plotting adjustments. Sequence diagram for propagated custom plot output stemssequenceDiagram
participant User
participant anota2seqRun
participant anota2seqPerformQC
participant anota2seqResidOutlierTest
participant anota2seqAnalyze
participant PDF
User->>anota2seqRun: anota2seqRun(fileStem)
anota2seqRun->>anota2seqPerformQC: anota2seqPerformQC(fileStem)
anota2seqPerformQC->>PDF: pdf(fileStem_plot_filename.pdf)
anota2seqRun->>anota2seqResidOutlierTest: anota2seqResidOutlierTest(fileStem)
anota2seqResidOutlierTest->>PDF: pdf(fileStem_residual_filename.pdf)
anota2seqRun->>anota2seqAnalyze: anota2seqAnalyze(fileStem)
anota2seqAnalyze->>PDF: pdf(fileStem_rvm_fit_filename.pdf)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="R/anota2seqMethods.R" line_range="296-298" />
<code_context>
+ )
+ directOut$singleRegMode <- "background"
+
+ directOut$singleRegMode[rownames(directOut) %in%
+ rownames(object@selectedTranslation@selectedRvmData[[selContrast]])
+ [object@selectedTranslation@selectedRvmData[[selContrast]][,"singleRegMode"] == "translation"]] <- "translation"
+
+ directOut$singleRegMode[rownames(directOut) %in%
</code_context>
<issue_to_address>
**issue (bug_risk):** The `singleDf` output leaves every identifier classified as `background` because `directOut` receives default numeric row names (`1:n`) while the selected result tables are indexed by gene identifiers; the `%in%` checks therefore never match and the assigned `translation`/`buffering` regulatory modes are lost.
**Triggers:** When `anota2seqGetOutput(..., output = "singleDf")` is used after regulatory-mode analysis.
**Suggested fix:** Set `rownames(directOut) <- rownames(object@dataP)` or match against `directOut$identifier` instead of `rownames(directOut)`.
</issue_to_address>
### Comment 2
<location path="R/anota2seqRun.R" line_range="73-85" />
<code_context>
## If parameters are specified check them
if(!is.null(thresholds)){
+
+ ## reset parameter list
+ parameters <- list(minSlopeTranslation = NULL,
+ maxSlopeTranslation = NULL,
+ minSlopeBuffering = NULL,
+ maxSlopeBuffering = NULL,
+ maxPAdj = NULL,
+ maxP = NULL,
+ minEff = NULL,
+ deltaPT = NULL,
+ deltaTP = NULL,
+ deltaP = NULL,
+ deltaT = NULL)
+
+
for(paramNames in 1:length(thresholds)){
</code_context>
<issue_to_address>
**issue (bug_risk):** Supplying any non-NULL `thresholds` list resets every unspecified filtering parameter to `NULL`, replacing the normal defaults such as `maxPAdj = 0.15` and the slope limits; a partial thresholds list therefore silently disables the omitted filters instead of retaining their defaults.
**Triggers:** When `anota2seqRun` is called with a partial `thresholds` list, such as only `maxPAdj`.
**Suggested fix:** Initialize the threshold list with the same default values used when `thresholds` is NULL, then overwrite only the supplied entries.
</issue_to_address>
### Comment 3
<location path="R/anota2seqPerformQC.R" line_range="196" />
<code_context>
if(useRVM==TRUE & onlyGroup==FALSE){
message("\tUsing RVM for omnibus interaction statistics\n")
- jpeg(paste(fileStem, "_rvm_fit_for_interactions.jpg", sep=""), width=800, height=400, quality=100)
+ pdf(paste(fileStem, "_rvm_fit_for_interactions.pdf", sep=""), width=8, height=4)
par(mfrow=c(1,2))
anota2seqPlotIGFit(intResidMS, intResidDf[1], qqName="Fit for interactions")
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing a stem containing a new subdirectory, such as `results/ANOTA2SEQ`, causes `pdf()` to fail because none of these functions creates the parent directory before opening the output device; the advertised custom-subdirectory workflow therefore errors when the directory does not already exist.
**Triggers:** When `fileStem` contains a directory that has not been created in advance.
**Suggested fix:** Create the parent directory before opening plots, or validate and document that callers must create it first.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: R/anota2seqMethods.R:298, R/anota2seqRun.R:85, R/anota2seqPerformQC.R:196
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+296
to
+298
| directOut$singleRegMode[rownames(directOut) %in% | ||
| rownames(object@selectedTranslation@selectedRvmData[[selContrast]]) | ||
| [object@selectedTranslation@selectedRvmData[[selContrast]][,"singleRegMode"] == "translation"]] <- "translation" |
There was a problem hiding this comment.
issue (bug_risk): The singleDf output leaves every identifier classified as background because directOut receives default numeric row names (1:n) while the selected result tables are indexed by gene identifiers; the %in% checks therefore never match and the assigned translation/buffering regulatory modes are lost.
Triggers: When anota2seqGetOutput(..., output = "singleDf") is used after regulatory-mode analysis.
Suggested fix: Set rownames(directOut) <- rownames(object@dataP) or match against directOut$identifier instead of rownames(directOut).
Comment on lines
+73
to
+85
| ## reset parameter list | ||
| parameters <- list(minSlopeTranslation = NULL, | ||
| maxSlopeTranslation = NULL, | ||
| minSlopeBuffering = NULL, | ||
| maxSlopeBuffering = NULL, | ||
| maxPAdj = NULL, | ||
| maxP = NULL, | ||
| minEff = NULL, | ||
| deltaPT = NULL, | ||
| deltaTP = NULL, | ||
| deltaP = NULL, | ||
| deltaT = NULL) | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): Supplying any non-NULL thresholds list resets every unspecified filtering parameter to NULL, replacing the normal defaults such as maxPAdj = 0.15 and the slope limits; a partial thresholds list therefore silently disables the omitted filters instead of retaining their defaults.
Triggers: When anota2seqRun is called with a partial thresholds list, such as only maxPAdj.
Suggested fix: Initialize the threshold list with the same default values used when thresholds is NULL, then overwrite only the supplied entries.
| if(useRVM==TRUE & onlyGroup==FALSE){ | ||
| message("\tUsing RVM for omnibus interaction statistics\n") | ||
| jpeg(paste(fileStem, "_rvm_fit_for_interactions.jpg", sep=""), width=800, height=400, quality=100) | ||
| pdf(paste(fileStem, "_rvm_fit_for_interactions.pdf", sep=""), width=8, height=4) |
There was a problem hiding this comment.
issue (bug_risk): Passing a stem containing a new subdirectory, such as results/ANOTA2SEQ, causes pdf() to fail because none of these functions creates the parent directory before opening the output device; the advertised custom-subdirectory workflow therefore errors when the directory does not already exist.
Triggers: When fileStem contains a directory that has not been created in advance.
Suggested fix: Create the parent directory before opening plots, or validate and document that callers must create it first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi,
This PR addresses two of small adjustment regarding how plots are saved:
Custom output paths (fileStem): Added the fileStem argument to anota2seqResidOutlierTest and passed it through anota2seqRun. This allows saving all outputs into custom subdirectories (e.g. fileStem = "results/ANOTA2SEQ") instead of writing directly to the working directory with hardcoded names.
Vector PDF format: Switched the few remaining .jpeg/.jpg outputs in anota2seqPerformQC, anota2seqResidOutlierTest, and anota2seqAnalyze to .pdf so that all generated figures in the package consistently use vector format and editable if needed for publication.
Updated the relevant .Rd documentation files.
These changes were mainly personal preferences that suited my workflow better, so feel free to adapt or ignore them if not relatable.
-- okey I am confused now. I had only two small commits!
I have attached a git patches
0001-Add-a-fileStem-parameter-to-allow-customization-of-o.patch
0002-refactor-switch-plot-output-format-from-JPEG-to-PDF-.patch
anota2seq_filestem_pdf_support.patch