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?
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: