Conversation
…ed existing method.
| group_vars = NULL, | ||
| proximity_metric = "join_count_z", | ||
| p_adjust_method = c("bonferroni", "holm", "hochberg", "hommel", "BH", "BY", "fdr"), | ||
| diff_trehsold = 0.1, |
There was a problem hiding this comment.
I wonder if the default threshold is perhaps a bit too strict? Many of the changes we see are quite small but significant. Maybe the FBS team has some input hree?
There was a problem hiding this comment.
Yeah I think you might be right. I just used the default in FindMarkers, but that's for log2FC differences which are probably greater than differences in medians.
There was a problem hiding this comment.
I think something like 0.01 could be suitable?
There was a problem hiding this comment.
Based on my little experience so far, when we get a diff_median < 0.1, it is considered quite low. But this number is not log2FC as ludvig mentioned.
There was a problem hiding this comment.
The main speed up that you see with FindMarkers comes from the filters used and this particular one is quite effective. Setting it to 0.1 will make the function feel a lot faster :-)
maxkarlsson
left a comment
maxkarlsson
left a comment
There was a problem hiding this comment.
Looks great! I have some comments on some parameters that would be great to discuss.
| #' @noRd | ||
| .finalize_da_results <- function(res_list, p_adjust_method, group_vars = NULL) { | ||
| bind_rows(res_list) %>% | ||
| group_by(pick(all_of(c("target", group_vars)))) %>% |
There was a problem hiding this comment.
Should we really perform p-value adjustment per group? I know there are often different opinions about this - but the effect here would be lower p-values --> more sensitivity and potential false positives. What is your take?
There was a problem hiding this comment.
Yeah that's a very good question. I was hoping you'd bring this up. I guess it depends on what you do with the results.
If you split the tests across many groups (e.g. cell types), you will most likely lose a bunch of hits if you adjust across all tests. In practice, I think you would most likely focus on a single cell type at the time, and then it might be better to adjust only within that cell type population.
Let's say we run the differential test between stimulated and control in 10 cell types, but we are only interested in B cells. Alternatively, we subset the Seurat object to only include B cells and then run the test we would get very different adjusted p-values for the B cell population. Both approaches are valid. I think there's a risk that you'll get very few significant hits if you do the testing across 10 cell type populations.
What I do think we could do is to only group by the optional group_vars when doing the adjustment. Currently, if you have more than 1 target, the adjustment will be done per target (e.g. stimulated1 and stimulated2 against the reference). In practice, you would probably evaluate the results within the context of both stimulation conditions, so we should do the adjustment across all conditions.
There was a problem hiding this comment.
Sounds good! I guess it is always possible for a user to override the p-value adjustment after the fact if they disagree
There was a problem hiding this comment.
it is a balance, indeed. Adjusting across all groups seems sometimes very harsh; often resulting in nothing being significant. On the other hand, I agree it is is the most correct adjustment. Perhaps with the increased filtering that comes with these updates we also soften the effect of the adjustment at bit? I would tend to keep it as it is now but evaluate this? Open for other ideas though, it's a tricky one.
There was a problem hiding this comment.
In our particular case we will also have the terrible situation of p >> n when dealing with proximity scores, and the p variables aren't independent from each other and the n cells aren't independent either! So talking p-values is kinda statistically fudgy regardless of adjustment. Whatever is a better user experience could take precedence perhaps.
There was a problem hiding this comment.
Yeah I feel like they are mostly there fore convenience to filter the data. Not sure what the best solution is.
There was a problem hiding this comment.
I would think the adjustments in general should be done globally for a given hypothesis test
There was a problem hiding this comment.
I also agree, but the problem with proximity scores is that we are comparing so many pairs of proteins potentially across many different groups, so the p-value adjustment will remove a lot of hits. I think it comes down to what we want users to get. Should they get more results to evaluate further?
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6ec5436. Configure here.
| if (method == "seurat") { | ||
| proximity_data <- proximity_data %>% | ||
| compute() %>% | ||
| ProximityScoresToAssay() |
There was a problem hiding this comment.
Seurat path ignores proximity metric
High Severity
The default method = "seurat" path calls ProximityScoresToAssay() without values_from = proximity_metric, so the matrix always pivots log2_ratio. The requested metric is only written into data_type, so non-default metrics are never tested and results are mislabeled.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6ec5436. Configure here.
| if (length(features) == 0) { | ||
| cli::cli_warn("No features pass min_diff threshold; returning empty data.frame") | ||
| return(fc_results[features, ]) | ||
| } |
There was a problem hiding this comment.
Empty filter results crash analysis
High Severity
When no features pass min_pct, min_diff_pct, or diff_threshold, .wilcox_de_test returns a data frame without p_val. The Matrix method then reads de_results$p_val as NULL while building the results tibble, which errors and aborts the whole run.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6ec5436. Configure here.
| digits = 3 | ||
| ) | ||
| data_1 <- sparseMatrixStats::rowMedians(object[, cells_1, drop = FALSE]) | ||
| data_2 <- sparseMatrixStats::rowMedians(object[, cells_2, drop = FALSE]) |
There was a problem hiding this comment.
Missing sparseMatrixStats install guard
Medium Severity
The default seurat path calls sparseMatrixStats::rowMedians directly, but expect_sparseMatrixStats() was added and never used. Since sparseMatrixStats is only in Suggests, a missing install yields a raw namespace error instead of the package’s usual install guidance.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6ec5436. Configure here.
Description
This PR adds a new
DifferentialProximityAnalysis.Matrixmethod for sparse matrices. The method can be invoked fromDifferentialProximityAnalysis.Seuratby settingmethod = "seurat"(default).This new method pivots the long formatted proximity table into a sparse matrix and uses
FindMarkersto compute the test statistics. The main difference is that missing observations are set to 0, meaning that each group will have a value for all cells in that group. This should be much more robust to ensure that the results reflect population differences and not differences between smaller subsets. This strategy can also pick up differences where one of the two test groups has no observations, which we previously missed.This PR also refactors the existing
DifferentialProximityAnalysis.data.framemethod (invoked by settingmethod = "legacy"on Seurat objects) and adds a few minor improvements for speed optimization and pre-filtering similar to whatFindMarkersdoes.Fixes: PNA-2835
Type of change
How Has This Been Tested?
TODO
PR checklist:
Note
High Risk
Default Seurat differential proximity now uses zero-imputed wide matrices and different defaults (
log2_ratio,min_cells_per_group), which can change scientific results versus the legacy path; new remotes/suggested packages affect installs.Overview
Differential proximity analysis gains a new
DifferentialProximityAnalysis.Matrixmethod and a defaultmethod = "seurat"on Seurat objects: long proximity tables are pivoted to a wide sparse matrix (missing pairs as 0), then tested with Wilcoxon logic aligned to Seurat/FindMarkers(diff_threshold,min_pct,min_diff_pct), using presto when available and sparseMatrixStats for medians. The prior table-based Wilcoxon path remains asmethod = "legacy".The data.frame method is refactored onto shared helpers (group keys, filtering, progress, p-value adjustment), uses faster
min_cells_per_groupfiltering via counts +semi_join, and updates outputs (e.g. drops duplicate method string, addsna.omit). Seurat loading now goes throughProximityScoreswith optionallazy,min_exp_join_count, and metric-type filtering before branching on method.Breaking / API: default
proximity_metricislog2_ratio(wasjoin_count_z);min_n_obsis renamedmin_cells_per_groupacross DPA/DCA and proximity (new default 10 on proximity). New suggested deps: sparseMatrixStats, presto (Remotes). Tests and roxygen are updated for the new behavior and result columns (pct_tgt/pct_refvs legacyn_ref/statistic/auc).Reviewed by Cursor Bugbot for commit 6ec5436. Bugbot is set up for automated code reviews on this repo. Configure here.