Add `fct_sort` function #156

russHyde · 2019-01-08T13:55:59Z

fct_sort takes a factor or character-vector (implictly converted to factor) and reorders the levels of that factor using a user-specified function .fun.

Code added to R/sort.R, unit tests into tests/testthat/test-fct_sort.R.

An example showing how to use fct_sort to sort number-containing character levels by the contained number.

Ensures that the values in the factor levels are unchanged by the sorting function, and uses ... to pass further arguments for the sorting function.

`fct_sort` takes a factor or character-vector (implictly converted to factor) and reorders the `levels` of that factor using a user-specified function `.fun`. Code added to `R/sort.R`, unit tests into `tests/testthat/test-fct_sort.R`. An example showing how to use `fct_sort` to sort number-containing character `levels` by the contained number.

hadley · 2019-01-19T16:30:59Z

Fixes #117

hadley · 2019-01-19T16:31:43Z

R/sort.R

+
+  old_levels <- levels(f)
+  new_levels <- .fun(old_levels, ...)
+  stopifnot(


Would you mind making this a slightly friendlier function using the style guide at http://style.tidyverse.org/error-messages.html ?

Added more informative messages to indicate:

when .fun returns with something other than a vector

when the sorted-levels are of different length from the input-levels

when the sorted-levels contains at least one level that is absent from the input-levels

hadley · 2019-01-19T16:32:34Z

R/sort.R

@@ -0,0 +1,38 @@
+#' Automatically sort factor levels according to a user-defined function


Maybe remove "Automatically"?

The description now starts #' Sort factor levels ...

hadley · 2019-01-19T16:34:30Z

R/sort.R

+#' # naive alphanumeric sorting "1" < "10" < "2"
+#' fct_sort(chr_fac, sort)
+#'
+#'  # number-based alphanumeric sorting "1" < "2" < "10"


I think this is maybe a bit complicated for an example? Maybe just do something with alphabetical sorting? And maybe sample() to show random reordering?

I replaced the example. The new examples includes

alphabetical-sort,

alphabetical-decreasing-sort (equiv to fct_rev)

alphabetical sort with an out-of-order baseline level (equiv to fct_relevel)

sampling from the levels

jennybc · 2019-01-20T00:08:46Z

The name fct_sort() feels potentially misleading to me, as it's not clear if it acts on the factor (what I would assume) or its levels (what actually happens).

Worth contemplating: could this be achieved this by making fct_relevel() more capable? I.e. make it work more like purrr::set_names(), which can accept a vector of names, individual names, or a function or formula.

If that's a no-go, I still think it's worth trying to think of a better name here.

huftis · 2019-01-20T16:14:14Z

The name fct_sort() feels potentially misleading to me, as it's not clear if it acts on the factor (what I would assume) or its levels (what actually happens).

The same is true for other forcats functions, like fct_rev() and fct_shuffle().

But perhaps fct_sort_levels() would be work? It doesn’t really take any longer time to write if you have auto-completion enabled.

russHyde · 2019-01-20T17:00:40Z

Totally agree with @jennybc and @huftis that a better name should be applied. Happy to rewrite as fct_sort_levels.

I didn't consider modifying the fct_relevel formals, but that is a good proposal and would be possible, eg, as fct_relevel(.f, ..., after = 0L, .fun)

huftis · 2019-01-20T17:07:10Z

I didn't consider modifying the fct_relevel formals, but that is a good proposal and would be possible, eg, as fct_relevel(.f, ..., after = 0L, .fun)

I would prefer having a separate fct_sort_levels() function. IMHO, having the sorting function as the second argument is much nicer. It’s faster and feels cleaner to write fct_sort_levels(x, mysort) than fct_relevel(x, .fun = mysort).

hadley · 2019-01-21T16:24:25Z

I think @jennybc was suggesting that fct_relevel(x, mysort) could be made to work.

Renamed man/R/tests files to correspond to the renamed function. Changed the description of `fct_sort_levels` to "Sort factor levels ..." following code review.

russHyde · 2019-01-24T14:20:10Z

I believe I have made all changes suggested by @hadley and @huftis .
The function has been renamed fct_sort_levels.

hadley · 2019-01-24T16:16:57Z

R/sort_levels.R

+#' Sort factor levels according to a user-defined function
+#'
+#' @param .f A factor (or character vector).
+#' @param .fun A function that will sort or permute the existing factor levels.


Suggested change

#' @param .fun A function that will sort or permute the existing factor levels.

#' @param .fun A function that will permute the existing factor levels.

@hadley : Given the emphasis on permutation in your edited version of the docs, would fct_permute_levels be a more appropriate name for the function?

Or, indeed, lvls_permute?

hadley · 2019-01-24T16:17:05Z

R/sort_levels.R

+#' @param .f A factor (or character vector).
+#' @param .fun A function that will sort or permute the existing factor levels.
+#'   It must accept one character argument and return a character argument of
+#'   the same length as it's input.


Suggested change

#' the same length as it's input.

#' the same length as its input.

hadley · 2019-01-24T16:17:38Z

R/sort_levels.R

+#' # Default (alphabetical) level-sorting:
+#' fct_sort_levels(medieval_experiment, sort)
+#'
+#' # Reversed ordering (equivalent to `fct_rev`):


It's not in general equivalent to fct_rev(), so I think it'd be better to just remove that comparison

hadley · 2019-01-24T16:17:59Z

R/sort_levels.R

+#' # Reversed ordering (equivalent to `fct_rev`):
+#' fct_sort_levels(medieval_experiment, sort, decreasing = TRUE)
+#'
+#' # Level-sorting with "Control" as the first level


And this seems inferior to fct_relevel() so I'd also drop it.

hadley · 2019-01-24T16:18:09Z

R/sort_levels.R

+#'   medieval_experiment,
+#'   function(x) c("Control", sort(setdiff(x, "Control"))))
+#'
+#' # Randomised sorting of the levels:


Suggested change

#' # Randomised sorting of the levels:

#' # Randomly permute the levels:

hadley · 2019-01-24T16:19:04Z

R/sort_levels.R

+      call. = FALSE
+    )
+  }
+  if (length(old_levels) != length(new_levels)) {


This feels a little overwrought to me. Doesn't fct_relevel() already give error messages?

I don't think it provdes an error in this setting - if passed a (vector) proper subset of a factor's levels, fct_relevel just pulls the subset to the start of the factor's levels:

fct_relevel(factor(letters[1:3]), c("b", "a")) [1] a b c Levels: b a c

(I thought the error messages might be overkill, but was trying to follow the examples in your style guide)

hadley · 2019-02-16T14:31:46Z

After thinking about @jennybc's comment more, I think this is best implemented as a special case of fct_relevel(). Your PR was very useful to help us think through the API and implementation — thanks!

russHyde · 2019-02-16T17:01:26Z

thanks

hadley changed the title ~~fix #117: add fct_sort function~~ Add fct_sort function Jan 19, 2019

hadley reviewed Jan 19, 2019

View reviewed changes

russHyde added 4 commits January 24, 2019 11:29

rename new function as fct_sort_levels

b75c18e

Renamed man/R/tests files to correspond to the renamed function. Changed the description of `fct_sort_levels` to "Sort factor levels ..." following code review.

update example: alphabetical-sort and sampling

55fb5d4

add informative error messages

67b1633

test: permutation .fun returns a vector

46dbd18

hadley reviewed Jan 24, 2019

View reviewed changes

hadley closed this Feb 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `fct_sort` function #156

Add `fct_sort` function #156

russHyde commented Jan 8, 2019

hadley commented Jan 19, 2019

hadley Jan 19, 2019

russHyde Jan 24, 2019

hadley Jan 19, 2019

russHyde Jan 24, 2019

hadley Jan 19, 2019

russHyde Jan 24, 2019

jennybc commented Jan 20, 2019 •

edited

Loading

huftis commented Jan 20, 2019

russHyde commented Jan 20, 2019

huftis commented Jan 20, 2019

hadley commented Jan 21, 2019

russHyde commented Jan 24, 2019

hadley Jan 24, 2019

russHyde Jan 24, 2019

russHyde Jan 24, 2019

hadley Jan 24, 2019

hadley Jan 24, 2019

hadley Jan 24, 2019

hadley Jan 24, 2019

hadley Jan 24, 2019

russHyde Jan 24, 2019

hadley commented Feb 16, 2019

russHyde commented Feb 16, 2019

		@@ -0,0 +1,38 @@
		#' Automatically sort factor levels according to a user-defined function

	#' @param .fun A function that will sort or permute the existing factor levels.
	#' @param .fun A function that will permute the existing factor levels.

	#' the same length as it's input.
	#' the same length as its input.

	#' # Randomised sorting of the levels:
	#' # Randomly permute the levels:

Add fct_sort function #156

Add fct_sort function #156

Conversation

russHyde commented Jan 8, 2019

hadley commented Jan 19, 2019

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

jennybc commented Jan 20, 2019 • edited Loading

huftis commented Jan 20, 2019

russHyde commented Jan 20, 2019

huftis commented Jan 20, 2019

hadley commented Jan 21, 2019

russHyde commented Jan 24, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented Feb 16, 2019

russHyde commented Feb 16, 2019

Add `fct_sort` function #156

Add `fct_sort` function #156

jennybc commented Jan 20, 2019 •

edited

Loading