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

Implement sec.axis for date, time, and datetime scales #2806

Merged
merged 6 commits into from Sep 18, 2018

Conversation

Projects
None yet
4 participants
@dpseidel
Member

dpseidel commented Aug 6, 2018

This PR implements secondary axes for date, time, datetime scales. Closes #2244.

@dpseidel dpseidel requested review from hadley and thomasp85 Aug 6, 2018

@thomasp85

Can you add an example showing a meaningful use of a Date transformation in the secondary axis?

if (!is.sec_axis(sec.axis)) stop("Secondary axes must be specified using 'sec_axis()'")
sc$secondary.axis <- sec.axis
}
sc

This comment has been minimized.

@thomasp85

thomasp85 Aug 10, 2018

Member

It seems this check is now so common that it should be factored out into a separate function

dpseidel added some commits Aug 17, 2018

merge master into sec_date
Merge remote-tracking branch 'upstream/master' into sec_date

# Conflicts:
#	NEWS.md
#	man/scale_date.Rd
#	tests/testthat/test-sec-axis.R
@dpseidel

This comment has been minimized.

Member

dpseidel commented Aug 18, 2018

Thanks @thomasp85.

4 things:

  • As you suggested, I pulled out the checks into a standalone function. The name may need some tweaking, currently set_sec_axis().
  • I've added two examples to the sec_axis() docs rather than to the scale_*_date() docs, just because that seemed more appropriate but I can move them around if people have strong feelings.
  • To come up with examples, I perused the original issue. One of the suggested use cases in #2244 was to have time since event on the secondary axis with datetime or date on the primary. This is not currently possible as a transformation per se because the api now sets the scale transformation of the secondary axis to match the primary's (rather than using the identity transformation); i.e. date scales expect dates and don't play nice with numeric or difftime objects. Regardless, this "time since event" axis can be implemented simply by passing a function to labels. Just thought it was worth noting this minor limitation of the current implementation.
  • currently sec.axis does not respect date specific arguments e.g. date_labels or date_breaks or date_minor_breaks, instead requiring the user to implement these formatting choices with the scales functions manually. Does it make sense to add these arguments/functionality to sec.axis directly?
@hadley

This comment has been minimized.

Member

hadley commented Sep 12, 2018

@thomasp85 can you please re-review?

@clauswilke clauswilke referenced this pull request Sep 12, 2018

Closed

Release ggplot2 3.1.0 #2890

25 of 25 tasks complete
merge master into sec_date
Merge remote-tracking branch 'upstream/master' into sec_date

# Conflicts:
#	NEWS.md
@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 17, 2018

@dpseidel For your last bullet point, do you mean adding these parameters to the function sec.axis() or adding a separate function, e.g. sec.axis.date(). The former seems a bad idea to me (since in most use cases the parameters would be useless) and the latter could be done but doesn't have to be part of this PR.

@dpseidel

This comment has been minimized.

Member

dpseidel commented Sep 17, 2018

@clauswilke I was intending to inquire about the former solution -- adding these as arguments to sec.axis() just as they are arguments to scale_*_datetime() -- but I can see that being unnecessary and undesirable. The same functionality is achievable if you know how to specify date formatting using scales functions, it's just not as clean/convenient.

Thanks for all your work pushing out this bug release! This last one of mine is pretty close, I'm just waiting on a final review. I've synced this branch with master now and may add more tests today to bring the codecov percentage up but otherwise, we should be near finished.

@clauswilke

This comment has been minimized.

Member

clauswilke commented Sep 17, 2018

@thomasp85 Any more comments from your end?

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Sep 17, 2018

I’m going to do a review this evening

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Sep 17, 2018

I think this is fine as-is. Agree that sec_axis() shouldn't get date specific arguments... Thanks for all your work on this Dana

@dpseidel dpseidel merged commit 4b880bb into tidyverse:master Sep 18, 2018

3 of 4 checks passed

codecov/patch 57.89% of diff hit (target 75.37%)
Details
codecov/project 77.8% (+2.42%) compared to 1e01e68
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment