train_continuous now preserves the class of the input #167

billdenney · 2018-08-06T20:07:17Z

dpseidel · 2018-08-06T20:51:42Z

NEWS.md

@@ -67,6 +67,9 @@

 * `rescale_mid()` now properly handles NAs (@foo-bar-baz-qux, #104).

+* `train_continuous()` now maintains the class of inputs when they are not numeric


@hadley is this going to make the release? I suspect it won't and this should actually be in its own new section up top:

# scales 1.0.0.9000 * ...

When this is resolved, I'll provide a new PR.

So no, this change will not be included in the 1.0.0 release. Please move this news bullet to the top with the new header I demonstrated above, and add your github handle and the issue number so that it follows style guidelines. No need to open a new PR, just make a new commit to your branch with the changes.

dpseidel · 2018-08-06T20:52:46Z

tests/testthat/test-scale.r

+test_that("train_continuous maintains class", {
+ expect_equal(train_continuous(1:5),
+ c(1, 5),
+ info="Normal numeric input works with train_continuous")


Can you please style your tests according to style.tidyverse.org? There should be space around each equal sign. The styler package can help with this.

billdenney · 2018-08-07T20:29:50Z

I think that I got everything with this update.

dpseidel · 2018-08-07T20:44:38Z

NEWS.md

@@ -1,3 +1,10 @@
+# scales 1.0.0.9000
+
+## Minor bug fixes and improvements


This subheading is unnecessary when listing changes to the dev version, it's added when organizing changes just before release. Please remove it. Thanks for your patience with all these little nits -- you caught us right at release week so versions in NEWS and DESCRIPTION, haven't been bumped yet.

No worries. I'd be making similar comments on people submitting PRs to my repositories.

…ontinuous_class_preserve # Conflicts: # NEWS.md

hadley

Sorry, just noticed I hadn't submitted my comments (I made them a while back)

hadley · 2018-08-20T11:55:51Z

R/scale-continuous.r

@@ -38,7 +38,11 @@ train_continuous <- function(new, existing = NULL) {
 if (is.factor(new) || !typeof(new) %in% c("integer", "double")) {
 stop("Discrete value supplied to continuous scale", call. = FALSE)
 }
- suppressWarnings(range(existing, new, na.rm = TRUE, finite = TRUE))
+ if (is.null(existing)) {


Can you explain why this is needed? I would expect range(NULL, x) to return the same type as x.

I would have expected that, too, but it doesn't. If it did, then this entire PR would probably be unnecessary.

> range(as.POSIXct("2018-01-31"), NULL) [1] "2018-01-31 EST" "2018-01-31 EST" > range(NULL, as.POSIXct("2018-01-31")) [1] 1517374800 1517374800 >

Oh this is related to the type nightmare that is c().

Can you add a comment to that affect? i.e. range(NULL, x) strips attributes

hadley · 2018-08-20T11:56:03Z

tests/testthat/test-scale.r

@@ -19,3 +19,24 @@ test_that("NA.value works for discrete", {
 expect_that(dscale(x, pal, "grey50")[1], equals("grey50"))
 expect_that(dscale(x, pal, "grey50")[5], equals("grey50"))
 })
+
+test_that("train_continuous maintains class", {


Can you please apply tidyverse style here too?

hadley · 2018-09-06T21:17:19Z

tests/testthat/test-scale.r

+test_that("train_continuous maintains class", {
+ expect_equal(train_continuous(1:5),
+ c(1, 5),
+ info = "Normal numeric input works with train_continuous")


Please remove all uses of info

I can do that, but why remove it? I find it to be useful to understand the underlying rationale for the test. Is there a preferred way to indicate rationale?

Rational should usually be encoded in the test name, or in a comment. In this case, I don't think that "Normal numeric input works with train_continuous" adds anything to my understanding of the test - it's implied by the test name that you'll be iterating through different types, and "working" is implied by expect_equal()

hadley · 2018-09-06T21:18:03Z

tests/testthat/test-scale.r

+})
+
+test_that("train_continuous with NULL input", {
+ expect_equal(train_continuous(new = NULL, existing = c(1, 5)),


It seems unnecessary to name both arguments to train_continuous()

…ontinuous_class_preserve

billdenney · 2018-09-07T17:56:47Z

@hadley, I think that I've resolved everything. There is a ≥ symbol apparently showing up somewhere in the documentation causing the manual build to fail, but I think that's unrelated to this PR

hadley

Thanks for all your work on this so far! A few more comments on the test organisation.

hadley · 2018-09-07T19:42:20Z

tests/testthat/test-scale.r

@@ -19,3 +19,36 @@ test_that("NA.value works for discrete", {
 expect_that(dscale(x, pal, "grey50")[1], equals("grey50"))
 expect_that(dscale(x, pal, "grey50")[5], equals("grey50"))
 })
+
+test_that(paste(


Can you please split this into two tests?

hadley · 2018-09-07T19:42:59Z

tests/testthat/test-scale.r

+ c(1, 5)
+ )
+ expect_error(train_continuous(LETTERS[1:5]),
+ regexp = "Discrete value supplied to continuous scale"


This regexp is a bit long - the point is to check that you're got the right error, not to validate the exact text of the error message.

But why is it in this test? It doesn't seem to be related to the topic of test.

Good point, I've separated it into another test (and while not technically related to this PR's main topic, it is related to increasing test coverage for train_continuous()).

hadley · 2018-09-07T19:43:12Z

tests/testthat/test-scale.r

+ "train_continuous maintains class with `existing=NULL`,",
+ "and changes class to the class of `existing` when not NULL."
+), {
+ expect_equal(


I think these can now fit on one line

hadley · 2018-09-07T21:59:42Z

Thanks!

As it breaks ggforce and doesn't fix the motivating problem described in #166.

train_continuous now preserves the class of the input

9765ba5

dpseidel reviewed Aug 6, 2018

View reviewed changes

Updated styling based on comments from @dpseidel

4993673

dpseidel reviewed Aug 7, 2018

View reviewed changes

billdenney added 2 commits August 7, 2018 16:52

Updated styling based on comments from @dpseidel

5674384

Merge branch 'master' of https://github.com/r-lib/scales into train_c…

ce4c989

…ontinuous_class_preserve # Conflicts: # NEWS.md

hadley reviewed Sep 6, 2018

View reviewed changes

billdenney added 2 commits September 7, 2018 13:35

Merge branch 'master' of https://github.com/r-lib/scales into train_c…

f6f163b

…ontinuous_class_preserve

Updated comments, styling, added one more test

7b44fdc

hadley reviewed Sep 7, 2018

View reviewed changes

Update test organization

c70ad00

hadley merged commit 2ccb2fb into r-lib:master Sep 7, 2018

billdenney mentioned this pull request Feb 25, 2019

Cannot Use ggplot2 with units r-quantities/units#164

Closed

hadley added a commit that referenced this pull request Nov 5, 2019

Revert #167

f69c4a7

As it breaks ggforce and doesn't fix the motivating problem described in #166.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

train_continuous now preserves the class of the input #167

train_continuous now preserves the class of the input #167

billdenney commented Aug 6, 2018

dpseidel Aug 6, 2018

billdenney Aug 7, 2018

dpseidel Aug 7, 2018

dpseidel Aug 6, 2018

billdenney commented Aug 7, 2018

dpseidel Aug 7, 2018 •

edited

Loading

billdenney Aug 7, 2018

hadley left a comment

hadley Aug 20, 2018

billdenney Sep 7, 2018 •

edited

Loading

hadley Sep 7, 2018

hadley Aug 20, 2018

hadley Sep 6, 2018

billdenney Sep 7, 2018

hadley Sep 7, 2018

hadley Sep 6, 2018

billdenney commented Sep 7, 2018

hadley left a comment

hadley Sep 7, 2018

hadley Sep 7, 2018

hadley Sep 7, 2018

billdenney Sep 7, 2018

hadley Sep 7, 2018

hadley commented Sep 7, 2018

		@@ -67,6 +67,9 @@

		* `rescale_mid()` now properly handles NAs (@foo-bar-baz-qux, #104).

		* `train_continuous()` now maintains the class of inputs when they are not numeric

		@@ -1,3 +1,10 @@
		# scales 1.0.0.9000

		## Minor bug fixes and improvements

train_continuous now preserves the class of the input #167

train_continuous now preserves the class of the input #167

Conversation

billdenney commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billdenney commented Aug 7, 2018

dpseidel Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billdenney Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billdenney commented Sep 7, 2018

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented Sep 7, 2018

dpseidel Aug 7, 2018 •

edited

Loading

billdenney Sep 7, 2018 •

edited

Loading