Fix zero HVG bug emerging when batch_key is given #1180
Conversation
| if min_disp is None: min_disp = 0.5 | ||
| if min_mean is None: min_mean = 0.0125 | ||
| if max_mean is None: max_mean = 3 | ||
| if max_disp is None: max_disp = np.inf |
There was a problem hiding this comment.
Two thoughts on this:
- Should this function throw an error if the user passes
n_topand one of these values? - These values should get documented. If we're not using
Noneas a sentinel for missing values, could these just be the argument defaults?
There was a problem hiding this comment.
- Should this function throw an error if the user passes
n_topand one of these values?
This is handled already with a "logg.info" line:
if n_top_genes is not None and not all(m is None for m in [
min_disp, max_disp, min_mean, max_mean
]):
logg.info('If you pass `n_top_genes`, all cutoffs are ignored.')
- These values should get documented. If we're not using
Noneas a sentinel for missing values, could these just be the argument defaults?
I actually like that they are None by default, it seems more consistent with the rest of scanpy. I added the defaults to the documentation.
What do you think?
There was a problem hiding this comment.
Default values
What I like about default arguments is that they get automatically documented and are probably right, while manually documented values can go out of date if the values change. I think we only use None as a default value when we want to be able to check if the user provided a value.
n_top_genes / value filters behavior
I think the behavior for the filters (i.e. max, min, and n_top_genes) should either work together or be mutually exclusive. If they're mutually exclusive, an error is better than a warning. Otherwise I think it's too easy to miss a warning message and think the code is doing something it's not.
For example, I think this line could select the top 100 genes that have a mean of at least 2:
sc.pp.highly_variable_genes(adata, min_mean=2, n_top_genes=100)If this goes in a script in a pipeline, it'd be very easy not to see the logged message. Then the user would have the wrong idea of what their output means. I think failing fast is better in this case.
In addition, I'm not sure that it makes sense for n_top_genes to have a higher priority than the value filters.
|
Was there an issue that should be closed by this? |
No description provided.