Generalize tl.dendrogram function to either axis#2771
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2771 +/- ##
==========================================
+ Coverage 74.56% 74.65% +0.08%
==========================================
Files 115 115
Lines 12713 12783 +70
==========================================
+ Hits 9480 9543 +63
- Misses 3233 3240 +7
|
| Axis (0 or 1) along which to calculate the dendrogram. Pass either `dim` or `axis` but not both. | ||
| {n_pcs} | ||
| {use_rep} | ||
| var_names |
There was a problem hiding this comment.
Is it overkill to make this also generalize i.e., you can groupby on either axis and you can subset (i.e., what var_names does now) on the opposite axis?
There was a problem hiding this comment.
I will say this part kind of threw me given the title of the PR. I see what what is going on now, but would be curious your thoughts @flying-sheep. I would expect to do identical operations on either axis but again, maybe overkill
Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
for more information, see https://pre-commit.ci
|
OK, updated this so it follows the decision implemented in #1244 |
flying-sheep
left a comment
There was a problem hiding this comment.
OK, I checked this out and did some more stylistic changes myself.
The big things for me are:
-
Documentation:
The obs column(s) to use to group observations. Default is None.
What does
Nonemean? -
The corresponding
.plfunction: Examples look like this:>>> sc.tl.dendrogram(adata, groupby='bulk_labels') >>> sc.pl.dendrogram(adata, groupby='bulk_labels')
and there’s no guidance or example for leaving out
pl.
If people try this, it will throw an error:>>> sc.tl.dendrogram(adata) >>> sc.pl.dendrogram(adata)
So will this
>>> sc.tl.dendrogram(adata, axis='var') >>> sc.pl.dendrogram(adata, axis='var')
I think these things should work, or at least be very well documented
|
@flying-sheep the first is easy to fix. For 2. , it's not clear to me what you're asking for here. It's been a while since I worked on this. Am I supposed to import this function in the |
|
Hi Dan, about 1.: I’m asking you what the semantic meaning is 😄 about 2.: there are two functions called Also an important question: in |
Uh oh!
There was an error while loading. Please reload this page.