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

Fix color control of plotScoreHeatmap when 'normalize=TRUE' #259

Merged
merged 6 commits into from
Jan 5, 2024

Conversation

dtm2451
Copy link
Collaborator

@dtm2451 dtm2451 commented Jan 2, 2024

Per #197

Bug Fix: This PR ensures that when users supply their own color map to the plotScoreHeatmap() function, it will be used regardless of whether normalize was set to TRUE.

New Feature: Also gives users expanded control over their legend by newly allowing override of SingleR's default strategies for filling the pheatmap inputs breaks, legend_breaks, and legend_labels.

ToDo:

  • add test
  • double-check the other pheatmap inputs for similar overrides, and fix if makes sense
  • roxygenize

@dtm2451
Copy link
Collaborator Author

dtm2451 commented Jan 3, 2024

Double-checked other pheatmap inputs that we explicitly touch in the function. All the ones already recorded as inputs are good now, but there 3 currently touched "siltently": breaks, legend_breaks, legend_labels.

If I remember correctly, the plan was to give all of these as explicit inputs so that they could be documented for the user, and I've done that in my last commit.

Also, I might need to rely on you @LTLA to get the updated man pages built. I'm having a bit of trouble getting a proper SingleR dev environment set up at the moment. (Bioconductor hosted devel docker image fails at installing Rcpp, so... can't install SingleR or SingleR deps.) Still working on a solution here, but I can't run R CMD CHECK, roxygen2::roxygenize(), or even test things locally in the meantime.

@dtm2451 dtm2451 requested a review from LTLA January 5, 2024 17:07
@LTLA
Copy link
Collaborator

LTLA commented Jan 5, 2024

LGTM, I just did some minor editing of the doc strings.

@LTLA
Copy link
Collaborator

LTLA commented Jan 5, 2024

I'm just going to merge and ignore the CI issues; can't do much about those.

@LTLA LTLA merged commit 35dbc66 into master Jan 5, 2024
1 of 2 checks passed
@LTLA LTLA deleted the heatmap-color branch January 5, 2024 23:51
Comment on lines 371 to 373
default_legend_breaks <- c(-abs.max, abs.max, length.out = 3)
default_legend_breaks <- c(-abs.max, abs.max)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, your bugfix here went the opposite way of what I'd have intended to do. I would have left the length.out and swapped back to the original seq() had I caught it....

Ultimately though, 🤷, new version just removes a middle label in the legend, which was always at zero. Now only the ends of the legend will be labeled but that's totally fine!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ha, I was wondering which way it should go. Ended up just flipping a coin in my head.

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.

2 participants