Allow setting one of the limits of scale #684

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@jimhester
Contributor

Implements setting of only one limit of a continuous scale by specifying
NA for the limit which you don't want to set. See #557.

I had to use NA instead of NULL to implement this, as is.null() is not
vectorized, a vector with null at any location is TRUE.

@jimhester jimhester Allow setting one of the limits of scale
Implements setting of only one limit of a continuous scale by specifying
NA for the limit which you don't want to set.
b87418b
@wch
Collaborator
wch commented Oct 10, 2012

Nice, this is a useful feature. I've made some comments on the code.

@wch wch commented on an outdated diff Oct 10, 2012
@@ -52,7 +52,7 @@ ylim <- function(...) {
limits <- function(lims, var) UseMethod("limits")
limits.numeric <- function(lims, var) {
stopifnot(length(lims) == 2)
- if (lims[1] > lims[2]) {
+ if (sum(is.na(lims)) == 0 &lims[1] > lims[2]) {
@wch
wch Oct 10, 2012 Collaborator

This is a bit cleaner IMO, and the && is more appropriate:

if (all(!is.na(lims)) && lims[1] > lims[2])

(edited this to use all instead of any)

@wch wch commented on an outdated diff Oct 10, 2012
@@ -292,7 +292,9 @@ scale_limits <- function(scale) {
#' @S3method scale_limits default
scale_limits.default <- function(scale) {
- scale$limits %||% scale$range$range
+ if(!is.null(scale$limits))
@wch
wch Oct 10, 2012 Collaborator

Stylistic stuff: I think this if should have curly braces and there should be spaces after commas. It would also be helpful to have a comment explaining how this works.

In case you're wondering why all the ggplot2 code isn't like this, it's because some of it is old code, written in a different style.

@jimhester
Contributor

I agree with all of your comments, the new commit should fix them. Also thanks for pointing out the any function, I was not aware of it, and it makes the code much clearer, I always thought the sum(logicals) idiom seemed hacky. Thanks for the comments!

@jimhester
Contributor

Is there anything else you need from this pull request in order to merge it? I would be happy to make any changes needed.

@wch
Collaborator
wch commented Dec 28, 2012

Hi Jim - at the moment I'm working on other projects besides ggplot2, but when I get back to it I'll be able to take a closer look at the pull request. Offhand it looks fine, but merging changes like this requires some careful consideration to make sure it that it's done right, and also that it doesn't introduce subtle bugs.

@hadley
Member
hadley commented Feb 24, 2014

Could you please rebase/merge against master, re-document with the development version of roxygen2 (install_github("klutometis/roxygen) and resubmit?

@hadley hadley closed this Feb 24, 2014
@jimhester jimhester referenced this pull request Feb 24, 2014
Merged

One limit #908

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment