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

Update UMAP faceting code #644

Merged
merged 19 commits into from
Jan 9, 2024
Merged

Conversation

sjspielman
Copy link
Member

Closes #637

This PR cleans up legend placement and makes associated changes for the faceted UMAPs where we highlight 1 cell type in each facet. Here's an overview of the changes:

  • I modified the lumping function to also wrap labels to 35 characters, which seems to be about right for the legend "slot" and associated spacing changes I made
    • Note that for the wrapping, I wrote a separate mutate() altogether since I think that's a bit more legible.
    • One wrinkle for wrapping is that, when there are two lines of text in the facet strip label, it slightly change the aspect ratio so it's not quite equal to 1. I don't think this a big deal though?
  • I fixed factor ordering - After running fct_lump(), we lost our original factor order and levels became alphabetical. Now, we are back to ordering by frequency, with "Unknown cell type" and "All remaining cell types" as the last two levels
  • I modified the legend text size and placement, which is now conditional
    • In a shocking twist of events, I was unable to help myself and did a %%3 anyways for determining legend placement, but there is still only 1 if statement so I think it's fine that I did that thing! I also covered the case of 1 cell type, which really should realistically never happen (the only circumstance I can think of is a method totally fails and assigns everything as unknown), but again, there's only 1 if :)
  • I enforced three columns for facet_wrap() which seemed a little safer to code explicitly given the legend placement choices
  • Note that dplyr:: had to be added into the umap function as described in this commit - 3fa66d0

Here are two rendered celltype reports:

@jashapiro
Copy link
Member

  • One wrinkle for wrapping is that, when there are two lines of text in the facet strip label, it slightly change the aspect ratio so it's not quite equal to 1. I don't think this a big deal though?

Can we add back aspect.ratio = 1?

@jashapiro
Copy link
Member

  • One wrinkle for wrapping is that, when there are two lines of text in the facet strip label, it slightly change the aspect ratio so it's not quite equal to 1. I don't think this a big deal though?

Can we add back aspect.ratio = 1?

Related to this: I think you want a bit of code to determine the height of these figures based on the # of cell types. (I think maybe that is why you got out of square?)

allyhawkins
allyhawkins previously approved these changes Jan 5, 2024
Copy link
Member

@allyhawkins allyhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these changes look good to me. I'm fine without the aspect ratio, because when I had it I was seeing some really small UMAPs that didn't look good. I think the scenarios where we have long cell type names > 35 characters is minimal too. But feel free to test it out.

@sjspielman
Copy link
Member Author

sjspielman commented Jan 5, 2024

Thanks for reminder about aspect ratio @jashapiro! Let me play around with this and height...

I think the scenarios where we have long cell type names > 35 characters is minimal too

(edit) Both* of the attached reports had this scenario - some of the singler cell type names are reaaallly long.

@sjspielman
Copy link
Member Author

I'm fine without the aspect ratio, because when I had it I was seeing some really small UMAPs that didn't look good.

Yeah, this is what I'm seeing, eg -
Screenshot 2024-01-05 at 2 52 22 PM

But more importantly, changing the chunk's plot size doesn't make a difference. It seems we'd have to modify the actual legend coordinates based on the plot's coordinates, which seems like more trouble than it's worth?

@sjspielman
Copy link
Member Author

I think you want a bit of code to determine the height of these figures based on the # of cell types. (I think maybe that is why you got out of square?)

All the UMAP chunks have height/width of 9, and cellassign + singler both had the same number of cell types (n=8). So, I think it's a product of the strip taking up 2 lines?

@allyhawkins
Copy link
Member

But more importantly, changing the chunk's plot size doesn't make a difference. It seems we'd have to modify the actual legend coordinates based on the plot's coordinates, which seems like more trouble than it's worth?

I would agree that I don't think it's worth it and I think you could leave it the way it is. The only other thing I would suggest if you really want to change it is just setting the figure height and width based on the number of cell types.

@jashapiro
Copy link
Member

But more importantly, changing the chunk's plot size doesn't make a difference. It seems we'd have to modify the actual legend coordinates based on the plot's coordinates, which seems like more trouble than it's worth?

This is because you kept the bottom right positioning though, right? If the legend were positioned by the top left corner, it would fix this, wouldn't it?

@jashapiro
Copy link
Member

I actually think keeping the aspect ratio fixed is pretty important, otherwise you get skewing between different plots.

@jashapiro
Copy link
Member

All the UMAP chunks have height/width of 9, and cellassign + singler both had the same number of cell types (n=8). So, I think it's a product of the strip taking up 2 lines?

I was specifically thinking of submitter plot where you only have 6. It becomes too long, so those aren't square.

@sjspielman
Copy link
Member Author

Ok, I think I've gotten there -

  # Determine legend placement based on total_celltypes
  if (total_celltypes %% 3 != 0 & total_celltypes != 1) {
    faceted_umap <- faceted_umap +
      theme(
        legend.position = c(0.68, 0.33),
        legend.justification = c("left", "top"),
        legend.title.align = 0.5,
        # use slightly smaller legend text, which helps legend fit and prevents
        #  long wrapped labels from bunching up
        legend.text = element_text(size = rel(0.85)), 
        aspect.ratio = 1
      )
  } else {
    faceted_umap <- faceted_umap +
      theme(
        legend.position = "bottom", 
        aspect.ratio = 1
      )
  }

All of them look -

Screenshot 2024-01-05 at 3 18 38 PM

@sjspielman
Copy link
Member Author

Next up, going to modify figure size params.

@sjspielman sjspielman dismissed allyhawkins’s stale review January 5, 2024 20:22

changes are happening

@sjspielman
Copy link
Member Author

I think setting size conditionally is a bit of a pain because we have to both contend with whether the legend is inset or on the bottom, as well as the number of cell types. So before I launch into that, what if we just move to 8x8 all around instead of 9x9...? Just make things a little less domineering.

For example -
celltypes_supplemental_report.html.zip

@jashapiro
Copy link
Member

You can do it in one case statement, something like:

fig_height <- dplyr::case_when(
  n_types <= 2 ~ 3, 
  n_types <= 3 ~ 4, 
  n_types <= 5 ~ 6, 
  n_types <= 6 ~ 7, 
  .default = 9
)

But maybe still not worth it.

@sjspielman
Copy link
Member Author

Getting closer (arrived?). Found a bug too for n=2 while exploring - legend has to be on the bottom here, since even though ncol = 3, there won't be a 3rd panel! I also killed the legend for n=1.

Reports for each N<=6 (see submitter UMAPs):

1-submitter.html.zip
2-submitter.html.zip
3-submitter.html.zip
4-submitter.html.zip
5-submitter.html.zip
6-submitter.html.zip

Copy link
Member

@allyhawkins allyhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My biggest comment is that the 8x8 doesn't look square to me. I would go back to 9x9 as the default so we have square UMAPs, at least in the case of cell type labels that are not on the longer end.

legend.text = element_text(size = rel(0.85)),
aspect.ratio = 1
)
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just add a comment here that this is specifically for when n = 2 that because we have < 3 columns, we need the legend on the bottom?
Maybe another comment for each of the if conditions would be helpful to say why each decision was made here.

n_celltypes <= 3 ~ 4,
n_celltypes <= 5 ~ 6,
n_celltypes <= 6 ~ 7,
.default = 8
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you setting width? I think we want it to be square and they are not square right now, even if they don't have long cell type names.
I think I would go back to the default being the 9x9.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are you setting width?

I just kept it fixed at 8; because of the aspect ratio, the whole space should only be taken up if needed, is what I saw while exploring. But will bump to 9 and explore a bit more!

@sjspielman
Copy link
Member Author

I have explored much more!

  • Width and height are now both dynamically set, using test celltypes that have long names to ensure spacing works out ok
  • I aligned figures in the center. At first I tried to left-align them all, but this didn't work as expected which was disappointed. So, center was as close as I got.

Here's a set of reports across number of cell types: reports-across-ncelltypes.zip

Copy link
Member

@allyhawkins allyhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused as to why you don't like sticking with making each plot 3x3 so the default is 9x9? Is something not looking right? Because the plots used to look much more square then they do now.

templates/qc_report/celltypes_qc.rmd Outdated Show resolved Hide resolved
templates/qc_report/celltypes_qc.rmd Outdated Show resolved Hide resolved
@sjspielman
Copy link
Member Author

I'm a little confused as to why you don't like sticking with making each plot 3x3 so the default is 9x9? Is something not looking right? Because the plots used to look much more square then they do now.

I think the problem is me 😬 ! I'm sure I've been focusing more on overall size than overall squareness... I'll do another round shortly!

@sjspielman
Copy link
Member Author

Noting that I downloaded a ruler app which actually works really well, and I measured all the panels for different N's. Each panel is indeed square; aspect.ratio = 1 is working as expected. Any appearance of non-square-ness is a visual artifact maybe from the facet labels. Either way, I'll make those changes to hopefully get the overall appearance looking more square.

sjspielman and others added 2 commits January 9, 2024 10:50
Co-authored-by: Ally Hawkins <54039191+allyhawkins@users.noreply.github.com>
Co-authored-by: Ally Hawkins <54039191+allyhawkins@users.noreply.github.com>
@sjspielman
Copy link
Member Author

Some images for your perusal @allyhawkins, let me know how you feel about these dimensions:

9"x9" for 3x3 grids, with and without wrapped labels.

9x9 - wrapped labels
9x9 - not wrapped labels

Three versions of a 5-facet plot:

8"x7" (WxH)

8x7

9"x7" (WxH)

9x7

9"x6" (WxH)

9x6

@allyhawkins
Copy link
Member

Okay I think I know why they aren't looking square to me. By using aspect.ratio = 1, the actual panel is square, but the addition of the label makes it look elongated. I'm attaching a copy of what was in the report until these changes, and to me they look visually more appealing, but if we want to keep aspect.ratio = 1, then I think I would choose the 9x9 and the 9x6 but move the legend up a little bit.
Screenshot 2024-01-09 at 10 44 56 AM

@sjspielman
Copy link
Member Author

sjspielman commented Jan 9, 2024

Okay I think I know why they aren't looking square to me. By using aspect.ratio = 1, the actual panel is square, but the addition of the label makes it look elongated.

Yup, that's the culprit! It's the labels.

but if we want to keep aspect.ratio = 1, then I think I would choose the 9x9 and the 9x6 but move the legend up a little bit.

It seems like we do for overall consistency across all report UMAPs. I'll set those width/heights!

edit see next comment!
But, in terms of moving the legend up, I agree, but I don't think we can do it without getting overly into the weeds. The reason that the legend is so low in the example "loooong" cell type images I have is because the legend position is based around the center of the legend itself. The fact that all the text is wrapped changes the overall legend dimensions and it ends up being lower. There is no way to get that legend higher while ensuring the other legends in the report don't then overlap with the facet above. Does my description make sense and did I understand your comment correctly?

@sjspielman
Copy link
Member Author

Oh wait actually, the positioning is actually fixable - I was wrong about the wrapping being the culprit, it's the fact that there aren't three rows. I think it's ok to add another if here. Incoming.

@sjspielman
Copy link
Member Author

Ok, how does it look??

celltypes_supplemental_report.html.zip

Copy link
Member

@allyhawkins allyhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the 🎢, but looks good to me!

@sjspielman
Copy link
Member Author

Sorry for the 🎢

never apologize for making me stay in ggplot2 land 😉 !

@sjspielman sjspielman merged commit b3cf819 into development Jan 9, 2024
3 checks passed
@sjspielman sjspielman deleted the sjspielman/637-faceted-umap-legend branch January 9, 2024 18:23
allyhawkins pushed a commit that referenced this pull request Jan 25, 2024
@allyhawkins allyhawkins mentioned this pull request Jan 31, 2024
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

Successfully merging this pull request may close these issues.

3 participants