Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisit the facet umap function #637

Closed
sjspielman opened this issue Dec 22, 2023 · 10 comments
Closed

Revisit the facet umap function #637

sjspielman opened this issue Dec 22, 2023 · 10 comments
Assignees

Comments

@sjspielman
Copy link
Member

See original comment: #635 (comment)

The legend placement is not sufficiently general for the plots we'll be using it for, so we'll need to revisit this code.

  • We don't lump cell types for submitters, so the legend needs to be placed "normally" outside the margins
  • For singler and cellassign, the positioning may also be dependent on text length
@sjspielman
Copy link
Member Author

We should also figure out if we really do or do not want theme(aspect.ratio = 1) in these plots.

@jashapiro
Copy link
Member

jashapiro commented Dec 22, 2023

In my opinion, we probably should lump submitter annotations for these plots too (move the lumping code into this function?).

For the text length problem, we may be able to solve it with modifying legend placement to something like:

legend.position = c(.66, .33),
legend.justification = c("left", "top"),

This assumes that the individual plots are square-ish, and there are 8 of them, arranged in a 3x3 grid which we should be trying to achieve.

@jashapiro
Copy link
Member

We should also figure out if we really do or do not want theme(aspect.ratio = 1) in these plots.

I think we do, in general, but it will require that we get the dimensions of the plot as a whole to match: if we do 3X3, we need the plot to be pretty square. If we have more than the 8 plots, then we will need a matching aspect ratio (and to deal with the legend differently).

One other option is to move some of the theme settings back out of the function that we are using... We can always add them back on a per-plot basis.

faceted_umap(...) + theme(...)

@sjspielman sjspielman self-assigned this Jan 3, 2024
@sjspielman
Copy link
Member Author

In my opinion, we probably should lump submitter annotations for these plots too (move the lumping code into this function?).

Noting that I was wrong - we do already lump submitter cell types; it's just that my brain was toast the day before break 😬 . The code lumps/mutates at across(ends_with("_celltype_annotation")), which includes all the annotation columns (singler, cellassign, and submitter).

So, we'll want to count the final number of cell types and determine appropriate layout from there to achieve some kind of 3x3 grid. I might vote something like this for N<=8, where dashed-line boxes are panels that may or may not be there depending on the actual N, and the lines with pink dots are legends. The yellow text is a loose concept of where to place the legend -

Extrapolating, we can use %%3 to conditionally handle legend placement:

  • if N == 1 | N%%3 == 0, legend goes on the bottom
    • Or, for N==1, unlike my drawing, we can put the legend on the right
  • If N %%3 == 2, legend goes at 0.66
  • If N %%3 == 1, legend goes at 0.33

The only time things would get out of handle with a 3-column layout is if there are a lot of clusters. Perhaps we cap the situation at like N=15, and change the ncol for anything bigger than that?

I'll note also I previously brought up the idea that we might not need a legend at all since the facets are all labeled anyways. This was nixed at the time, but maybe it's more of a contender now?

Note I also intend to use str_wrap() to play around with overall label sizes.

Tagging @allyhawkins @jashapiro for post-winter-break thoughts!

@allyhawkins
Copy link
Member

  • if N == 1 | N%%3 == 0, legend goes on the bottom

    • Or, for N==1, unlike my drawing, we can put the legend on the right
  • If N %%3 == 2, legend goes at 0.66

  • If N %%3 == 1, legend goes at 0.33

This approach seems reasonable to me.

The only time things would get out of handle with a 3-column layout is if there are a lot of clusters. Perhaps we cap the situation at like N=15, and change the ncol for anything bigger than that?

I think we had decided that we would use N = 7 for lumping? That's our default and we don't actually change it in the workflow so I would not anticipate having any more than 7 cell types + 1 for all other cell types in any of the UMAPs. If you really wanted to be careful we could add a check to the lump_celltypes function to make sure N<=7. So I really think we just need to consider the situations where we have less than 8 categories. I wouldn't worry about the situations where we have more since that won't happen.

I'll note also I previously brought up the idea that we might not need a legend at all since the facets are all labeled anyways. This was nixed at the time, but maybe it's more of a contender now?

I also would be okay with removing the legend. The reason I do like having it is because the colors are different for each cell type so then users would know what color is expected in that panel.

@sjspielman
Copy link
Member Author

Yes - n=7 for lumping but then the extra "all other cell types" brings us to 8 total. I think we're saying the same thing?

@allyhawkins
Copy link
Member

Yes, we are. But I don't know why we would ever have a scenario with N=15.

@sjspielman
Copy link
Member Author

sjspielman commented Jan 3, 2024

But I don't know why we would ever have a scenario with N=15.

It definitely wouldn't happen with cell types, but possibly with clusters.

EDIT: Ah, nope! We don't use the facet function for clusters. So indeed it wouldn't happen!!

@sjspielman
Copy link
Member Author

@jashapiro and I had a quick chat about this on Slack. Notes -

  • We'd probably like to avoid excessive logic since it's preferable to keep code changes more minimal
  • Let's make our lives easier:
    • If 7 or 8 categories, legend goes in the bottom right at ~0.66 (depending on fiddling). We expect this to be the vast majority situations anyways. Cell type methods indeed enjoy assigning cell types.
    • Anything else, just put the legend on the bottom.

@sjspielman
Copy link
Member Author

Closed by #644

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants