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

Report domains for several transformations #394

Merged
merged 3 commits into from
Nov 1, 2023

Conversation

teunbrand
Copy link
Contributor

Briefly, ggplot2 uses the domain to figure out in what range it can ask a transformation to compute breaks after scale expansion.
Having a default domain of c(-Inf, Inf) works for most transformations, but not all.
In particular the boxcox_trans() doesn't really work as intended as a consequence of having the default domain. While trying to fix that, I decided to also add domains for asn_trans() and probability transformations.

A short demo of the issue and proposed fix:

library(scales)

# Note: I'm not giving the scale negative values, some become negative through expansion
demo_continuous(1:10, trans = boxcox_trans(2))
#> scale_x_continuous(trans = boxcox_trans(2))
#> Error: boxcox_trans must be given only positive values. Consider using modulus_trans instead?

# With this PR:
devtools::load_all("~/packages/scales/")
#> ℹ Loading scales

demo_continuous(1:10, trans = boxcox_trans(2))
#> scale_x_continuous(trans = boxcox_trans(2))

Created on 2023-07-25 with reprex v2.0.2

@thomasp85
Copy link
Member

Can you update to remove conflict?

@teunbrand
Copy link
Contributor Author

Certainly. Also realised that log1p_trans() should have a domain.

@teunbrand
Copy link
Contributor Author

Failure on devel seems related to some unicode <-> grid interaction? Seems unrelated to the 3 PRs you asked me to update.

@thomasp85 thomasp85 merged commit 9fff26c into r-lib:main Nov 1, 2023
11 of 12 checks passed
@thomasp85
Copy link
Member

Yeah, don't worry about that

@teunbrand teunbrand deleted the trans_domains branch November 1, 2023 10:12
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