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

Feature: set_channel() sets or skips NA values #40

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

zeehio
Copy link

@zeehio zeehio commented Sep 20, 2022

Sorry for so many pull requests.

On top of #39 (please review them in order), this pull request deals with the behaviour of missing values in functions that modify channels.

When a replacement value is NA, two behaviours may be expected depending to what we understand a NA modification to be:

  • The modification is invalid so we set the colour to NA (e.g. set_channel("black", "r", NA) returns NA)
  • The modification is missing, so we ignore the modification and leave the colour as it was (e.g. set_channel("black", "r", NA) returns "black")

Both interpretations make sense to me.

The current specification handles the case when colour is missing (the na_value is used). However it does not specify the behaviour when value is missing. Currently, in all cases but one (a bug, see below) the behaviour is to set the corresponding colour to NA (the "modification is invalid" intepretation).

In this pull request we introduce the skip_na_values argument. If FALSE (the default), when the replacement value is NA we set the colour to NA. If TRUE, when the replacement value is NA we leave the colour unmodified.

While working on this feature I fixed an inconsistency in how the alpha channel handled missing values. If you believe it only makes sense to keep one interpretation, please tell me and I will close this pull request and replace it with another one that:

  • Specifies in the documentation the behaviour with a NA in value
  • Addresses the following issue
library(farver)
set_channel("red", "alpha", NA_integer_)
#> [1] "#FF000000"
set_channel("red", "alpha", NA_real_)
#> [1] "#FF0000"

Created on 2022-09-20 with reprex v2.0.2

Thanks, and again apologies for all the extra work!

@thomasp85
Copy link
Owner

Hi @zeehio

Thanks for all the work you are doing here and in the ggplot2 repo. It is much appreciated. I'm currently fully occupied with preparing the next ggplot2 release so I do not have time to sit down with all the PRs right now, but will once ggplot2 has been released. Feel free to continue the work in the meantime or let it sit and we can have a talk about your proposals.

Best
Thomas

@zeehio
Copy link
Author

zeehio commented Sep 20, 2022

Hi @thomasp85,

Thanks for your reply. No worries and no hurries. I don't want to bury you under pull requests 😃 .

I would be very happy to talk with you. Feel free to send me an email at sergioller@gmail.com to schedule it at your convenience. Thank you and good luck with the ggplot2 release 👍

Best,
Sergio

When a replacement value is `NA`, two behaviours may be expected:

- Set the colour to `NA`
- Ignore the modification and leave the colour as it was

Here we introduce skip_na_values. If `FALSE` (the default), when the
replacement value is NA we set the colour to NA.

If `TRUE`, when the replacement value is NA we leave the colour
unmodified.
So I can depend on these patches on my Remotes:
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.

None yet

2 participants