Fix HVFInfo not able to handle multiple layers and HVF methods #241
Conversation
There was a problem hiding this comment.
Thanks for this one @cyrillustan 🙌 Your fix does appear to resolve the reported issues with Seurat::VariableFeaturePlot:
- VariableFeaturePlot() fails if VariableFeatures() is run twice with different selection methods seurat#9210
- cannot find dispersion value in HVFInfo(seu) seurat#9215
However, I'm not sure that HVFInfo.StdAssay is behaving correctly just yet. To help you along, I've introduced a comprehensive set of unit tests for the method in the test-hvf-info branch. If you rebase your branch onto mine, you'll be able to use the tests as your acceptance criteria.
I made the following assumptions about the updated behavior of HVFInfo:
methodis a required parameter (i.e. an error is thrown if it's not explicitly set)- The default value of
layerisNULLwhich will take the value ofDefaultLayer(assay) - If a specified
method,layercombo does not exist, an error is raised, empty dataframes are never returned
I'm more than happy to hash out the details in this thread if you disagree with any of these, or any of the other behavior laid out in test_assay5.R 🤓 Otherwise, let me know if you need any help with the rebase or subsequent refactor 👍
In the meantime, please update the description of this PR per my previous comment.
| @@ -972,16 +972,14 @@ HVFInfo.StdAssay <- function( | |||
| object, | |||
| method = NULL, | |||
| status = FALSE, | |||
| layer = NULL, | |||
| layer = NA, | |||
There was a problem hiding this comment.
I think the intent of setting layer = NULL as the default was to emulate the behavior of Layers.StdAssay and use the value of DefaultLayer(object). The downside of this is that you might get an error with layer = NULL that you can avoid with layer = NA.
I'm okay with updating the default behavior since it's more flexible and the default layer will typically be the first one anyway. Just make sure to reflect the change in the new tests 👌
| @@ -972,16 +972,14 @@ HVFInfo.StdAssay <- function( | |||
| object, | |||
| method = NULL, | |||
There was a problem hiding this comment.
HVFInfo.Assay requires method to be provided. While we could try to infer the method from the value of layer, I think it's best to make it a required parameter. This would technically be a breaking change, but the current behavior is inconsistent, so I don't think it can/should be relied on.
R/assay5.R
Outdated
| # if there is multiple methods, use the first one | ||
| method <- method %||% names(x = methods)[length(x = methods)] |
There was a problem hiding this comment.
Ignoring the comment, I think your intent was:
method <- method %||% names(vf.methods.layers)[length(vf.methods.layers)]Which will return the last element in names(vf.methods.layers if method is NULL.
I'm not quite sure how methods found it's way in here but it's a built-in function for listing the generics associated with a given class. In this case, names(methods)[length(methods)] is equivalent to NULL[length(NULL)] so if method is NULL, it won't be updated.
That said, if we make method a required parameter, we can forgo this logic altogether.
R/assay5.R
Outdated
| warning("multiple layers are identified by ", | ||
| paste0(layer, collapse = ' '), | ||
| "\n only the first layer is used") |
There was a problem hiding this comment.
If we're going to raise this warning, it's probably worth mentioning the method too.
R/assay5.R
Outdated
| warning("multiple layers are identified by ", | ||
| paste0(layer, collapse = ' '), | ||
| "\n only the first layer is used") | ||
| layer <- layer[1] |
There was a problem hiding this comment.
nit:
| layer <- layer[1] | |
| layer <- layer[1L] |
For Issue #104