Revision of qcscore_model_spe branch - #64
Conversation
BenedettaBanzi
commented
Jul 23, 2026
BenedettaBanzi
commented
- Fixed some issues with QS computation with custom formula;
- updated QC functions documentation;
- added more customazable settings to plot functions;
- function name standardization to Quality Score (QScore);
- updated vignettes.
…lected metrics. Function name standardization to QScore. plotCellsFovs with distinct parameters for points and FOV number sizes. Updated documentation and vignettes.
There was a problem hiding this comment.
Pull request overview
This PR updates SpaceTrooper’s Quality Score (QS) APIs and documentation, including renaming/standardizing “QCScore” to “QScore”, expanding plot customization controls, and refreshing vignettes/man pages for the new interface.
Changes:
- Renames QS-related functions/outputs (e.g.,
computeQCScore()→computeQScore(),QC_score→QScore) and updates package docs/vignettes accordingly. - Adds/extends support for user-specified QS model formulas and updates QS model training/application documentation.
- Expands plotting customization parameters (e.g., point size/alpha and label sizes) and updates associated docs/vignettes.
Reviewed changes
Copilot reviewed 14 out of 32 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| vignettes/SpaceTrooper_utilities.Rmd | Updates utilities vignette for QScore naming and adds custom-formula example. |
| vignettes/RNA_tutorial_vignette.rmd | Updates RNA tutorial to use renamed QS APIs and updated plotting args. |
| vignettes/Protein_tutorial_vignette.Rmd | Updates protein tutorial to use renamed QS APIs and updated plotting args. |
| README.md | Updates quickstart example to computeQScore()/computeQScoreFlags(). |
| R/spatialQCPlots.R | Adds plot customization parameters and refactors plotting function signatures. |
| R/readMerfish.R | Updates warning message wording for Quality Score terminology. |
| R/QC.R | Renames/updates QS computation pipeline, formula handling, model application helper, and extensive roxygen updates. |
| NEWS.md | Updates release notes for 1.1.8 changes. |
| NAMESPACE | Updates exports to new function names (adds .applyQScoreModel, swaps QCScore exports for QScore). |
| man/trainModel.Rd | Updates documentation wording for ridge logistic regression model fitting. |
| man/spatialPerCellQC.Rd | Updates parameter descriptions and expanded behavior documentation. |
| man/plotZoomFovsMap.Rd | Documents new plotZoomFovsMap arguments and naming. |
| man/plotCellsFovs.Rd | Documents new plotCellsFovs arguments and naming. |
| man/getModelFormula.Rd | Updates usage to metricList (but needs consistency fixes). |
| man/dot-computeXenMerTrainSet.Rd | Documentation wording update for internal helper. |
| man/dot-computeCosmxTrainSet.Rd | Documentation wording update for internal helper. |
| man/dot-computeCosmxProteinTrainSet.Rd | Documentation wording update for internal helper. |
| man/dot-computeBorderDistanceCosMx.Rd | Documentation wording updates for FOV terminology. |
| man/dot-checkSkw.Rd | Documentation wording update for internal helper. |
| man/dot-applyQScoreModel.Rd | Adds documentation for .applyQScoreModel() (example currently inconsistent with actual API). |
| man/computeTrainDF.Rd | Updates docs to QScore_train naming and other wording. |
| man/computeThresholdFlags.Rd | Updates docs wording and clarifies outputs. |
| man/computeSpatialOutlier.Rd | Updates docs wording and clarifies behavior. |
| man/computeQScoreFlags.Rd | Adds documentation for computeQScoreFlags(). |
| man/computeQScore.Rd | Adds documentation for computeQScore(). |
| man/computeQCScoreFlags.Rd | Removes deprecated/renamed QCScoreFlags man page. |
| man/computeQCScore.Rd | Removes deprecated/renamed computeQCScore man page. |
| man/computeOutliersQScore.Rd | Renames and updates outlier-computation documentation for QScore. |
| man/computeLambda.Rd | Updates usage to new signature (but needs argument/example consistency fixes). |
| man/checkOutliers.Rd | Updates docs wording and naming to Quality Score. |
| man/applyQCScoreModel.Rd | Removes deprecated/renamed applyQCScoreModel man page. |
| DESCRIPTION | Bumps version to 1.1.8. |
Files not reviewed (18)
- man/checkOutliers.Rd: Generated file
- man/computeLambda.Rd: Generated file
- man/computeQScore.Rd: Generated file
- man/computeQScoreFlags.Rd: Generated file
- man/computeSpatialOutlier.Rd: Generated file
- man/computeThresholdFlags.Rd: Generated file
- man/computeTrainDF.Rd: Generated file
- man/dot-applyQScoreModel.Rd: Generated file
- man/dot-checkSkw.Rd: Generated file
- man/dot-computeBorderDistanceCosMx.Rd: Generated file
- man/dot-computeCosmxProteinTrainSet.Rd: Generated file
- man/dot-computeCosmxTrainSet.Rd: Generated file
- man/dot-computeXenMerTrainSet.Rd: Generated file
- man/getModelFormula.Rd: Generated file
- man/plotCellsFovs.Rd: Generated file
- man/plotZoomFovsMap.Rd: Generated file
- man/spatialPerCellQC.Rd: Generated file
- man/trainModel.Rd: Generated file
Comments suppressed due to low confidence (4)
man/computeLambda.Rd:18
computeLambdadocumentation is inconsistent with the new signaturecomputeLambda(modelMatrix, trainDF): it still documents amodelFormulaargument and the examples callcomputeLambda(df_train, modform). This will mislead users and fail during example checks.
This issue also appears on line 39 of the same file.
man/computeLambda.Rd:42
- The example still uses the old API (
getModelFormula(metadata(spe)$formula_variables)andcomputeLambda(df_train, modform)), but the function signatures were updated. This example will error if executed.
vignettes/SpaceTrooper_utilities.Rmd:513 - The vignette currently tells users the
I(abs(log2AspectRatio) * as.numeric(dist_border < 50))term "must have spaces around the*and<operators". Requiring exact whitespace makes the API brittle and is easy for users to get wrong; it’s better to document that whitespace is irrelevant (and ensure the implementation normalizes it).
R/QC.R:369 computeLambda()docs still say it "constructs the design matrix via model.matrix()", but the function now takesmodelMatrixas an argument and no longer builds it internally. This makes the description misleading.
#' Internally, the function:
#' constructs the design matrix via \code{model.matrix()},
#' runs k-fold cross-validation of ridge logistic regression using `cv.glmnet` with `alpha = 0`,
#' extracts and returns `ridge_cv$lambda.min`.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| df <- ctx$df; out_var <- ctx$out_var; tech <- ctx$tech | ||
|
|
||
| train_df <- computeTrainDF(df, out_var, tech, verbose) | ||
| model_formula <- getModelFormula(names(out_var)) |
There was a problem hiding this comment.
This is a wrong suggestion. model_formula should be built ALWAYS using the computed variables contained in out_var, not just when modelFormula is null. out_var is the vector of metrics resulting from the checks made by .prepQCContext. If the number of outliers is not enough, that metric is automatically discarded from the vector
There was a problem hiding this comment.
non è così, se fornisci un argomento (modelFormula), stai dando la possibilità di usarlo.
Altrimenti è inutile fornire tale argomento.
Quindi il funzionamento logico è:
se la formula in argomento è NULL, allora si usano le componenti che abbiamo impostato noi di default. Quindi questo è il comportamento standard suggerito dagli autori (noi).
In alternativa, se l'utente vuole sperimentare altre metriche, scrive una formula con queste metriche e la passa a modelFormula, che quindi andrà a sostituire la formula decisa da noi.
There was a problem hiding this comment.
Ma l'argomento infatti viene utilizzato! Viene usato in questa parte qui, per costruire la prima versione della formula custom che poi però deve essere controllata:
if (!is.null(modelFormula)) { model_formula <- modelFormula metricList <- attr(terms(as.formula(modelFormula)), "term.labels") metricList <- metricList[!grepl(":", metricList, fixed=TRUE)] if("I(abs(log2AspectRatio) * as.numeric(dist_border < 50))" %in% metricList) { metricList <- gsub("I\\(abs\\((log2AspectRatio)\\) \\* as\\.numeric\\((dist_border) < 50\\)\\)", "log2AspectRatio", metricList) } }
L'utente può scegliere che metriche mettere nella formula, ma non può mettere quello che vuole. Devono essere tra le metriche che noi abbiamo approfondito, come abbiamo illustrato nella supplementary note. Altrimenti che senso ha aver inserito una tabella di metriche e tutta la spiegazione sul perché le abbiamo scelte? Se avessimo dovuto lasciare all'utente il compito di cercarsi quali metriche combinare assieme, non avrebbe avuto senso pubblicare proprio SpaceTrooper.
model_formula <- getModelFormula(names(out_var))
Questa riga di codice ci deve essere sempre perché viene dopo questo:
ctx <- .prepQCContext(spe, metricList, verbose) df <- ctx$df; out_var <- ctx$out_var; tech <- ctx$tech
Il che significa che stiamo veramente dando all'utente la possibilità di togliere o mettere delle metriche nella sua formula, ma ci stiamo anche assicurando che la formula che usa rispetti i criteri che valgono per la formula di default e che devono valere sempre. Ossia:
- log2SignalDensity deve esserci per forza perché è il requisito minimo per calcolare il Quality Score;
- per ciascuna metrica ci deve essere un numero minimo di outlier pari allo 0.01% della numerosità del dataset;
- per dataset di Xenium e MERFISH/MERSCOPE non si deve usare il border effect.
Se non rispettiamo il primo criterio, non abbiamo la più pallida idea di quello che si ritrova in mano l'utente per fare QC, e mettere una metrica per cui non c'è un numero sufficiente di outlier, o il border effect per tecnologie in cui non lo abbiamo osservato, vuol dire addestrare il modello su una variabile che non è informativa per quel dataset, col rischio di andare a filtrare delle cellule su un criterio che è sbagliato.
There was a problem hiding this comment.
ok, capisco quello che dici.
A questo punto, dobbiamo però separare chiaramente libertà dell’utente e validazione del metodo.
Secondo me qui vanno distinti due problemi diversi.
Il primo è permettere davvero all’utente di usare una formula personalizzata. Se esiste un argomento chiamato modelFormula, la formula fornita deve essere quella effettivamente usata dal modello. Se l’utente sceglie una formula poco sensata, se ne assume la responsabilità; al massimo possiamo avvertirlo, ma non dovremmo sostituirla silenziosamente con una formula ricostruita da noi.
Con il codice attuale, invece, modelFormula viene usata soltanto per estrarre le metriche e poi viene sovrascritta da:
model_formula <- getModelFormula(names(out_var))Quindi l’utente non sta realmente scegliendo la formula, ma soltanto un sottoinsieme di variabili e combinato nel modo stabilito da noi (ad es imponendo che ci siano sempre gli interaction term).
Il secondo problema è fare dei controlli di validità. Questo è indipendente dal primo e può essere gestito separatamente verificando, per esempio:
- che le variabili indicate esistano in
colData; - che
log2SignalDensitysia presente, se la consideriamo un requisito strutturale del QScore; - che alcune metriche siano compatibili con la tecnologia;
- che le metriche validate da SpaceTrooper abbiano un numero sufficiente di outlier.
Le metriche fuori dalla whitelist potrebbero comunque essere accettate, producendo un warning:
unsupported <- setdiff(
all.vars(as.formula(modelFormula)),
validated_metrics
)
if (length(unsupported)) {
warning(
"The following predictors have not been validated by SpaceTrooper ",
"and are used at the user's own risk: ",
paste(unsupported, collapse = ", ")
)
}La scelta della formula dovrebbe quindi restare distinta dai controlli:
model_formula <- if (is.null(modelFormula)) {
getModelFormula(names(out_var))
} else {
modelFormula
}In sintesi: noi possiamo controllare che la formula sia applicabile e avvertire l’utente quando esce dal perimetro validato, ma non dovremmo modificarne automaticamente la struttura. Altrimenti modelFormula non è davvero una formula custom.
| #' @param formulaVars A named character vector mapping variable names | ||
| #' (e.g. `"log2SignalDensity"`, `"Area_um"`, etc.) to their corresponding | ||
| #' outlier label columns, typically from | ||
| #' `metadata(spe)$formula_variables`. |
| out_var <- metricList | ||
| if ("log2AspectRatio" %in% out_var) { | ||
| out_var[grep("log2AspectRatio", out_var)] <- | ||
| "I(abs(log2AspectRatio) * as.numeric(dist_border<50))" |
| #' spe_test <- applyQScoreModel( | ||
| #' spe=spe_test, | ||
| #' qcModel=qc_model, | ||
| #' scoreName="QC_score_transferred" | ||
| #' scoreName="QScore_transferred" | ||
| #' ) |
| spe_test <- applyQScoreModel( | ||
| spe=spe_test, | ||
| qcModel=qc_model, | ||
| scoreName="QScore_transferred" | ||
| ) |
| getModelFormula(metricList) | ||
| } | ||
| \arguments{ | ||
| \item{formulaVars}{A named character vector mapping variable names | ||
| (e.g. `"log2SignalDensity"`, `"Area_um"`, etc.) to their corresponding | ||
| outlier label columns, typically from | ||
| `metadata(spe)$formula_variables`.} |
| #' `~(log2SignalDensity + Area_um + I(abs(log2AspectRatio) * as.numeric(dist_border < 50)) + log2Ctrl_total_ratio)^2`. | ||
| #' When user-provided, the formula must follow the same default syntax and | ||
| #' removed (or added) terms should be written exactly as in the default formula, | ||
| #' e.g. `I(abs(log2AspectRatio) * as.numeric(dist_border < 50))` must have spaces | ||
| #' around the `*` and `<` operators. |
| computeQScore <- function(spe, bestLambda=NULL, modelFormula=NULL, verbose=FALSE) { | ||
| stopifnot(is(spe, "SpatialExperiment")) | ||
| if (dim(spe[,spe$total == 0])[2] != 0) { | ||
| warning(paste0(dim(spe[,spe$total == 0])[2], |
| #' @param trainDF `data.frame` | ||
| #' A data frame for training that must include: | ||
| #' A data frame for QS model training that must include: | ||
| #' Predictor columns: All columns referenced in the formula returned by `getModelFormula()`. | ||
| #' `qscore_train` A binary (0/1) response vector to be modeled. | ||
| #' @param modelFormula `character` |
| #' @description | ||
| #' Apply a previously trained QC score model to a new SpatialExperiment object. | ||
| #' Internal: applies a previously trained Quality Score model to a new SpatialExperiment object. | ||
| #' See details for important considerations when applying a model to a | ||
| #' different dataset. | ||
| #' @details |
|
@copilot based on your review, provide possible solutions to the problems in a different PR, and add the old computeQCScore function with the .Deprecated instruction based on the bioconductor guidelines present here: |
|
@drighelli I've opened a new pull request, #65, to work on those changes. Once the pull request is ready, I'll request review from you. |
| plotCellsFovs <- function(spe, sampleId=unique(spe$sample_id), | ||
| pointCol="firebrick", numbersCol="black", | ||
| alphaNumbers=0.8, fovDim=metadata(spe)$fov_dim, | ||
| size=0.05, alpha=0.8, | ||
| pointCol="firebrick", pointSize=0.05, | ||
| pointAlpha=0.8, numbersCol="black", | ||
| numberSize= 1, numbersAlpha=0.8, | ||
| fovDim=metadata(spe)$fov_dim, |
| map <- plotCellsFovs(spefovs, pointCol=mapPointCol, | ||
| numbersCol=mapNumbersCol, alphaNumbers=mapAlphaNumbers, | ||
| sampleId=NULL, size=csize, alpha=calpha, scaleBar=scaleBarMap) | ||
| pointSize=mapPointSize, pointAlpha=mapPointAlpha, | ||
| numbersCol=fovNumbersCol, numberSize=fovNumberSize, | ||
| numbersAlpha=fovNumbersAlpha, sampleId=NULL, scaleBar=scaleBarMap) |
| Type: Package | ||
| Title: SpaceTrooper performs Quality Control analysis of Image-Based spatial | ||
| Version: 1.1.7 | ||
| Version: 1.1.8 |
There was a problem hiding this comment.
@BenedettaBanzi never do the version bump together with all the modifications.
It requires a different commit.
| df <- ctx$df; out_var <- ctx$out_var; tech <- ctx$tech | ||
|
|
||
| train_df <- computeTrainDF(df, out_var, tech, verbose) | ||
| model_formula <- getModelFormula(names(out_var)) |
There was a problem hiding this comment.
ok, capisco quello che dici.
A questo punto, dobbiamo però separare chiaramente libertà dell’utente e validazione del metodo.
Secondo me qui vanno distinti due problemi diversi.
Il primo è permettere davvero all’utente di usare una formula personalizzata. Se esiste un argomento chiamato modelFormula, la formula fornita deve essere quella effettivamente usata dal modello. Se l’utente sceglie una formula poco sensata, se ne assume la responsabilità; al massimo possiamo avvertirlo, ma non dovremmo sostituirla silenziosamente con una formula ricostruita da noi.
Con il codice attuale, invece, modelFormula viene usata soltanto per estrarre le metriche e poi viene sovrascritta da:
model_formula <- getModelFormula(names(out_var))Quindi l’utente non sta realmente scegliendo la formula, ma soltanto un sottoinsieme di variabili e combinato nel modo stabilito da noi (ad es imponendo che ci siano sempre gli interaction term).
Il secondo problema è fare dei controlli di validità. Questo è indipendente dal primo e può essere gestito separatamente verificando, per esempio:
- che le variabili indicate esistano in
colData; - che
log2SignalDensitysia presente, se la consideriamo un requisito strutturale del QScore; - che alcune metriche siano compatibili con la tecnologia;
- che le metriche validate da SpaceTrooper abbiano un numero sufficiente di outlier.
Le metriche fuori dalla whitelist potrebbero comunque essere accettate, producendo un warning:
unsupported <- setdiff(
all.vars(as.formula(modelFormula)),
validated_metrics
)
if (length(unsupported)) {
warning(
"The following predictors have not been validated by SpaceTrooper ",
"and are used at the user's own risk: ",
paste(unsupported, collapse = ", ")
)
}La scelta della formula dovrebbe quindi restare distinta dai controlli:
model_formula <- if (is.null(modelFormula)) {
getModelFormula(names(out_var))
} else {
modelFormula
}In sintesi: noi possiamo controllare che la formula sia applicabile e avvertire l’utente quando esce dal perimetro validato, ma non dovremmo modificarne automaticamente la struttura. Altrimenti modelFormula non è davvero una formula custom.
| model_matrix <- model.matrix(as.formula(modelFormula), data=trainDF) | ||
| model_matrix <- .dropModelIntercept(model_matrix) | ||
| ridge_cv <- cv.glmnet(model_matrix, trainDF$qcscore_train, | ||
| computeLambda <- function(modelMatrix, trainDF) { |
There was a problem hiding this comment.
@BenedettaBanzi come mai hai cambiato qui?
ogni cambio di argomento diventa un "deprecated"
