Fix tick misalignment with secondary axes (#2978)

[dpseidel]

Simplification of the secondary axis in #2796 dropped the necessary range rescaling procedure to properly position secondary axis ticks relative to their primary axis. A partial reversion of #2796 , this PR reintroduces proper scaling while maintaining the transformation inheritance that currently supports sec.axis with date and datetime scales and proper breaks for log transformed scales.

This PR also reverts changes made in #2805, reversing the order in which the range of the secondary axis is sampled and inverted. For proper positioning of secondary ticks relative to the primary scale, the data range must be sampled before taking the inverse (or else the range may be too large to get a sufficiently fine sample for position matching, e.g. with log-transformed data). This reversion does reintroduce some spurious monotonicity test failures present in 3.0.0 which will be addressed in a separate issue.

I have added new tests to check positioning, not simply correct transformation. 2 new visual tests are added and 2 are updated. In some cases, I opted to use code from user reprexes for tests, but these could be simplified if preferred.

Fixes #2978

[dpseidel]

Updated my tests just now to use data_frame() to prevent further issues, however builds will continue to fail until #3003 is complete and merged.

[thomasp85]

I'll wait with this until #3003 is merged then but looks good in general. Do you have a fix for the monotonicity check in mind?

[dpseidel]

I have some ideas but haven't had a chance to work up a clean patch just yet, any ideas are most welcome.

The test will fail in cases when a user asks for a secondary axis for a power-transformed primary axis and data close to 0. In these cases, default expansion during plot build will mean the sample range may, after sampling and inversion, no longer be monotonic and will cause the plot to fail. Since the failures are due to the passing of expanded ranges to AxisSecondary break_info, if expand = c(0,0) is specified all spurious failures go away.

Reprex built with current PR:

library(tibble)
library(ggplot2)

data <- data_frame(
  x = seq(0, 1, length.out = 100),
  y = seq(0, 4, length.out = 100)
)

## transformation works without the secondary axis.... 
ggplot(data, aes(x, y)) +
  geom_line() + 
  scale_y_sqrt()

## failure with secondary
ggplot(data, aes(x, y)) +
  geom_line() + 
  scale_y_sqrt(sec.axis = dup_axis())

#> Error in f(..., self = self): transformation for secondary axes must be monotonic

## works with expand = c(0,0)
ggplot(data, aes(x, y)) +
  geom_line() + 
  scale_y_sqrt(sec.axis = dup_axis(), expand = c(0,0))

^{Created on 2018-11-16 by the reprex package (v0.2.1)}

[dpseidel]

Hey, coming back around to this -- @thomasp85 do you want to take a second look? All builds pass now.

[thomasp85]

Can you, just for my sanity, describe what is different here from my initial (buggy) version.

[dpseidel]

For your sanity and mine 😅

Your original sec.axis code was not buggy, instead some of the changes I implemented towards the end of my internship were. Quite. In fact this PR, reverts back to a lot of your original code and adapts it only slightly to play nice with the central change of #2796 (i.e. scale transformation inheritance by the secondary axis).

This PRs makes only two changes compared to your "original" break_info function, one at line 166 and at line 170. The lines of interest are embedded below for comparison, unfortunately I couldn't find an easy way to produce a diff between my fork and a specific commit on the master.

original:

ggplot2/R/axis-secondary.R

Line 128 in 4825054

new_range <- range(full_range, na.rm = TRUE)

currentPR:

ggplot2/R/axis-secondary.R

Line 166 in c036488

new_range <- range(scale$transform(full_range), na.rm = TRUE)

original:

ggplot2/R/axis-secondary.R

Lines 133 to 134 in 4825054

	old_val <- lapply(range_info$major_source, function(x) which.min(abs(full_range - x)))
	old_val <- old_range[unlist(old_val)]

currentPR, slightly combined :

ggplot2/R/axis-secondary.R

Line 170 in c036488

old_val <- old_range[unlist(lapply(range_info$major_source, function(x) which.min(abs(scale$trans$transform(full_range) - x))))]

The only difference in both places is a switch from full_range to scale$transform(full_range) to accommodate the primary transformation that is now inherited in the at L186 of the create_scale() function.

With the partial reversion and these two changes, this PR fixes the alignment issues while maintaining the primary transformation inheritance that supports correct break placement for transformed secondary axes and sec.axis with date and time scales.

However,

going through this fix, and especially in light of a related but new issue brought up yesterday, I wonder if we shouldn't take a step back and consider reverting #2796 and #2805 entirely. The changes made in #2796 set a new expectation that I should have realized would be potentially breaking: that the underlying trans of both axes now must match. This expectation was not imposed in the original api and although inheritance provides a straightforward way to incorporate sec axis for date and date time scales (#2806) in principle it really shouldn't be required there either.

The original motivation for my digging into AxisSecondary was #2729, improper sec.axis break placement with transformed scales; but a simpler fix for this can be achieved with a one line change to the init() functions of AxisSecondary, which is a more logical placement perhaps and does not require transformation inheritance. I've implemented the reversion and this minor patch on a separate branch here and included a temporary hack to preserve sec.axis functionality for datetime and time scales.

Obviously we don't want to break any existing functionality, and perhaps the shared transformation behavior is desirable but given the range of bugs my changes unwittingly introduced into 3.1.0, before deciding on a fix, I think we should discuss (and then clearly document) the expected functionality and limitations envisioned for sec.axis.

[dpseidel]

I'm going to close this now in lieu of #3040.

[lock]

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/