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 factor colour #131

Merged
merged 6 commits into from
Aug 5, 2024

Conversation

william-hutchison
Copy link
Contributor

If a factor is used to annotate the heatmap, the order of colours provided to palette will now match the order of the factor levels.

Previously, the colorRampPalette(palette_annotation$discrete[[.y]])(length(unique(.x))) %>% setNames(unique(.x)) was used to assign factor levels their colour, which does not consider factor level order.

This PR fixes issue #113

@stemangiola
Copy link
Owner

Amazing, remember that for each PR you should have unit test that proofs you fix. In you changed files I see a lot of plots that change color palette scheme. Is this related with you PR?

Also see what issues this PR can be linked to, I think there are couple of issues, and I also tried to solve this without success if I remember well.

@stemangiola stemangiola self-requested a review August 1, 2024 04:07
@william-hutchison
Copy link
Contributor Author

Hi Stefano,

I have linked the PR to issue #113 in the description, but don't think I have access to link using the actual development dropdown menu.

I can't quite remember why, but it seems factors previously always used a grey colour for one value and then rotated through the rest of the default palette. Maybe the grey colour was intended for NA values, but not being used because the levels were not correctly assigned. Whatever the cause, I think the new output is more in line with what the user might expect when using the default palette.

Thank you for the suggestion, I have added a unit test which explicitly tests for annotation tile factor level colour palette order.

@stemangiola stemangiola linked an issue Aug 2, 2024 that may be closed by this pull request
# If factor levels correctly interpreted, colour palette should read in order:
# "first_level", "second_level", "third_level", "fourth_level"

p =
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please add another test where you manipulate the factor order

levels = c( "second_level", "third_level", "fourth_level", "first_level")

And you test the effect

@stemangiola stemangiola merged commit 5d9409e into stemangiola:master Aug 5, 2024
2 of 3 checks passed
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.

Feature request: use named vectors for discrete palettes in tiles
2 participants