Fix tick misalignment and revert transformation inheritance in sec.axis (#2978) #3040
Conversation
…tity breaks function This "fix" breaks sec.axis functionality date and date time scales, and will re-introduce spurious monotonicity failures for some transformed scales with data values close to 0.
nearly imperceptible changes to the positioning of sec.axis
Merge branch 'sec_scale' into revert2796 # Conflicts: # R/axis-secondary.R # tests/testthat/test-sec-axis.R
|
Can we somehow make the monotonicity check only work on the unexpanded range? |
included theme_linedraw for easier identification of tick alignment
|
So yes, we can call the unexpanded range from |
|
So, as per discussion at tidy-dev-day @thomasp85 , I think we are done here. I've added some documentation about the limits to sec.axis with date time scales and significant tests. I considered adding a check (and tidy error) in the |
|
Hi, I had a feature on a package where the second Y-axis would be 1 - the first Y-axis value. So if the Y axis on the left went up from 0 to 1 in increments of 0.05, the second axis did the same, but in the reverse direction, from 1 to 0 in the same incremements. Would this feature still be available after this update? It was available before 3.1, but then it seems that a bug in 3.1 seems to have prevented this transformation which I coded as |
|
@Zadchow Yes, this feature is now supported again and I have added a test to ensure no future regressions. Thanks for your comment! @thomasp85 could you review this again when you get a chance? |
|
This fixed my issues with a secondary axis with sqrt transformation on a reversed axis, thank you very much! |
|
Thanks Dana |
|
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/ |
Version 3.1.0 introduced significant regressions in regards to the secondary axis API.
These regressions have been jointly documented in issue #2978 ad PR #3004 with reprexes but I will just briefly review them here.
changes made in Simplify sec.axis for proper transformed breaks #2796, eliminated the rescaling of the secondary axis to that of the primary axis,
introducing tick alignment issues when non-linear secondary transformations were called.
changes made in Fix incorrect monotonicity test failure in sec.axis #2805, reversed the order in which range transformation and sampling were completed, introducing poorly aligned ticks in the secondary axis in the case of non-linear transformations.
These two regressions taken together, break the expected sec.axis tick positioning for some custom transformation functions.
PR #3004 provided a fix for the first two issues, but would leave plots implementing independent primary and secondary scale transformations broken. In its stead, this PR reverts both 2796 and 2805 implementing a more conservative fix for the original motivating issue (log transformed breaks #2729) in the
init()function ofAxisSecondary.https://github.com/dpseidel/ggplot2/blob/fded720827358ad0012fa635a5af5292f45f4c1c/R/axis-secondary.R#L139
I believe this is the best course of action in light of the nature of all the regressions, but this fix opens two new issues:
1. Renewed spurious monotonicity test failures given power transformations with data near 0. (present in 3.0.0)
Upon digging into it, I realized the monotonicity issue is one we have chosen not to remedy in the past (#2008) simply offering a work-around: users should specify
expand = c(0,0). From a users perspective it is rather unexpected that a duplicate secondary axis would fail for a primary that does not, but because the range passed to the sec.axis is already expanded, there does not appear to be a simple solution.2. lost functionality for secondary date/datetime axes (datetime scales)
To preserve the current functionality of sec.axis with date and date time scales introduced in 3.1, I have opted for a bit of a hack: a conditional that passes the datetime transformation to the sec.axis scale preserving the current (inheritance) behavior in 3.1 only for date and datetime scales. I don't think this is a permanent solution and would appreciate advice or suggestion. At the very least we need to better document the expected functionality and limitations of sec.axis with all continuous scales, perhaps explicitly supporting more limited sec.axis functionality with datetime scales.
It seems to me that if we are preserving independent transformations for all other secondary axes, we should do so for date and datetime scales too. It's not implausible that users of datetime axes would want independent scales either: for example, one might want to create a "Time since event" secondary axis, which is currently only doable by passing custom labels to a sec.axis with a time/date transformation. However, the current rescaling framework, as well as the way init values are set via the primary scale, throws errors when POSIX values used with an identity transformation. Short of committing to a complete retooling, I did not find a way to work with POSIX objects in the current framework that would not require some special handling, at least to reformat labels, and this seemed, at least for now, the cleanest way to approach it with the obvious downfall that it makes independent axis transformations impossible. I can't immediately think of a use case where a nonlinear transformation would be appropriate with date time scale - so I don't forsee tick misalignment issues with this current patch. Regardless, please consider the current patch a place to start the discussion not necessarily an appropriate final solution.
Wrap up
In preparing this PR, I've reviewed all open issues regarding sec.axis and added a host of visual and other tests to prevent new regressions. Note that I have commented out the tests for the monotonicity issue which at this point still cause a failing build - if we opt not to fix this issue these can be removed. Any ideas, thoughts, or advice from others, especially the core-developers, are appreciated. Please let me know if I can clarify anything.
@thomasp85, I would specifically appreciate your comments/review when you get a minute. I'm not 100% happy with where this is at but I figure getting this open gives us a place to start from.
Fixes #2978.