Simplify sec.axis for proper transformed breaks #2796
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR simplifies the creation of secondary axes to use the transformation of the primary axis when setting breaks and labels rather than relying on an identity transform in all cases. This is a fix for #2729 which was caused by the identity trans calling
extended_breaks()
by default while the breaks on the primary axis were created bylog_breaks()
as is default for log-transformed axes.@thomasp85 As I mentioned this morning, my original fix just used a simple conditional to force log transformed axes to use
log_breaks()
but after you mentioned the identity trans this morning I decided to take another look at setting the new scale'strans
argument by the primary transformation. Ultimately this solves the problem at its source, and is a much more general fix. For instance,dup.axis()
should now respect any custom breaks function fed to a custom transformation used on the primary axis. Plus it drastically simplifies the code for building a secondary axis and makes implementation forscale_*_date()
/scale_*_datetime()
fairly straightforward. In fact I have already implemented sec.axis for these scales on a separate branch and will create a separate PR when this or some iteration of this is merged. Thanks again for making the time this morning, especially given my tardiness.I've added some new tests, examined numerous plots, and not yet found a case where this breaks anything or significantly changes behavior, but a careful review is warranted.