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

Correct domain calculation for compose_trans #408

Merged
merged 2 commits into from
Nov 6, 2023

Conversation

mjskay
Copy link
Contributor

@mjskay mjskay commented Nov 6, 2023

This PR corrects the domain calculations for compose_trans(), which appear to be incorrect.

The problem

Currently, compose_trans() pushes the domain of the first function through all transformations to calculate the domain. This procedure, when it works (i.e. if the range of each transformation in the chain is contained within the domain of the next one), yields the range of the composed set of transformations, not the domain.

This leads to incorrect domains; e.g.:

compose_trans("sqrt", "reverse")$domain
## [1] -Inf    0

The correct domain here is just the domain of sqrt(), which is c(0, Inf). We can see this by plotting it:

plot(compose_trans("sqrt", "reverse"), xlim = c(-1,4))
## Warning message:
## In trans$transform(x) : NaNs produced

image

This also leads to compose_trans() incorrectly erroring on valid compositions; e.g.:

compose_trans("reverse", "log")
## Error in `compose_trans()`:
## ! Sequence of transformations yields invalid domain
## Run `rlang::last_trace()` to see where the error occurred.

Yet, this should be a valid transformation, just with the domain c(-Inf, 0) instead of the original domain of c(0, Inf) (this also offers a solution to the question about reverse log in #393: the transformation is valid so long as the domain is correctly calculated).

The solution

This PR adjusts the domain calculation to push the first domain forward through the transformations, intersecting it with the domains of each transformation along the way, then pushing the resulting range back through the inverses to get the domain.

On the above two examples, with this PR, we have:

compose_trans("sqrt", "reverse")$domain
## [1]   0 Inf

and

compose_trans("reverse", "log")$domain
## [1]    -Inf -1e-100

@thomasp85
Copy link
Member

Thanks - merging your previous PR introduced a conflict. Can I get you to resolve that?

@thomasp85 thomasp85 added this to the scales 1.3.0 milestone Nov 6, 2023
@thomasp85 thomasp85 merged commit eba8e99 into r-lib:main Nov 6, 2023
12 checks passed
@mjskay mjskay deleted the compose_domain branch November 27, 2023 04:53
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