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 bug with continuous legends #119

Merged
merged 7 commits into from
May 23, 2023
Merged

Conversation

dan-knight
Copy link
Contributor

Description

In new versions of R (> R v4.3.0), lattice does not allow labels to be specified in a draw.colorkey without corresponding values for at. This change fixes BPG's call to draw.colorkey within legend.grob, placing labels equidistant along the range of continuous values.

Checklist

  • This PR does NOT contain PHI or germline genetic data. A repo may need to be deleted if such data is uploaded. Disclosing PHI is a major problem.

  • This PR does NOT contain molecular files, compressed files, output files such as images (e.g. .png, .jpeg), .pdf, .RData, .xlsx, .doc, .ppt, or other non-plain-text files. To automatically exclude such files using a .gitignore file, see here for example.

  • I have read the code review guidelines and the code review best practice on GitHub check-list.

  • I have set up or verified the main branch protection rule following the github standards before opening this pull request.

  • The name of the branch is meaningful and well formatted following the standards, using [AD_username (or 5 letters of AD if AD is too long)]-[brief_description_of_branch].

  • I have added the major changes included in this pull request to the NEWS under the next release version or unreleased, and updated the date. I have also updated the version number in DESCRIPTION according to semantic versioning rules.

  • Both R CMD build and R CMD check run successfully.

@dan-knight dan-knight added the bug Something isn't working label May 19, 2023
@dan-knight dan-knight requested a review from stefaneng May 19, 2023 21:33
@dan-knight dan-knight requested a review from a team as a code owner May 19, 2023 21:33
@dan-knight dan-knight requested a review from jarbet May 19, 2023 21:34
Copy link
Contributor

@stefaneng stefaneng left a comment

Choose a reason for hiding this comment

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

Do you have an example of this to test?

R/legend.grob.R Outdated
Comment on lines 149 to 151
boundaries <- seq(0, max.value, length.out = n.labels + 1);

sapply(
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent off here at least on github maybe just spaces/tabs mixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it! Should be fixed now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whitespace is still not good. There are a bunch of tabs before sapply

image

Copy link
Contributor Author

@dan-knight dan-knight May 23, 2023

Choose a reason for hiding this comment

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

Screen Shot 2023-05-23 at 9 47 25 AM
Seems to be fixed now. (Disregard the refactor - indentation should be the same before vs. after.)

@dan-knight
Copy link
Contributor Author

Do you have an example of this to test?

I actually found out about this issue because of a failing example from create.heatmap (the last one, continuous legend). This example is run with our R CMD check pipeline, so the passing check shows that it now runs without errors. Here's the plot output with the fix:

image

@dan-knight dan-knight merged commit b802fcf into main May 23, 2023
@dan-knight dan-knight deleted the danknight-fix-continuous-legend branch May 23, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants