Apply vctrs principles to `map()` and `modify()` #894

hadley · 2022-08-27T18:55:30Z

Fixes #435. Fixes #928.

For the purposes of discussion. If we agree that these semantics are what we want, need to:

Add news bullet
Implement map2_vec()
Implement pmap_vec()
Update modify

Fixes #435

lionel-

Another problem to solve is to generically map over classed lists like list_of() with full restoration behaviour. I think that's out of scope for map_vec() but could live in another function, perhaps list_map()?

R/map.R

tests/testthat/test-map.R

R/map.R

hadley · 2022-08-29T21:09:25Z

Updated the implementation (and tests) based on your comments. Do you think this looks good enough to finish off map2_vec() etc?

lionel-

LGTM!

Do we want to document this and other _vec variants in a different topic marked as experimental?

R/map.R

Conflicts: R/map.R man/map.Rd tests/testthat/_snaps/map.md

hadley · 2022-09-14T20:51:37Z

This will fail until it uses #942, but the implementation is now very simple/clear.

You might wonder why map_vec() gets its own function while map_c() does not. I think there are three reasons:

map_vec() is a real map in that map_vec(x, f)[[i]] == f(x[[i]])
We consider simplification to be an internal detail, and its done automatically in accumulate() and list_transpose()
We could one day implement this more efficiently, especially when .ptype is supplied.

And add missing .progress argument to map_vec(). And move map_vec() to correct location.

And extract out modify_where

And fix bugs thus revealed

hadley · 2022-09-15T22:01:35Z

Sorry that this has become such a monstrous PR, but you should be able to mostly ignore the changes to the map2 and pmap tests since I felt obliged to update them to at least cover a few of the key invariants.

The most important stuff to hit:

Are we still confident in map_vec() now that the implementation is so simple?
Have I reasoned through modify() and friends correctly?

R/map2.R

R/modify.R

lionel- · 2022-09-16T11:07:07Z

R/modify.R

+    vec_restore(out, .x)
+  } else if (vec_is(.x)) {
+    map_vec(.x, .f, ..., .ptype = .x)
+  } else if (is.null(.x) || is.list(.x)) {


It's a bit strange to go through that path for .x = NULL. Maybe branch into return(NULL)?

I had this originally, but it also felt weird — it is nice that NULL just falls out as a special case of a zero length list.

FWIW I also prefer the explicit is.null(x) branch

I'm going to leave it with list because (a) it's not that import and (b) it needs the same recycling behaviour as a zero-length list in modify2().

lionel- · 2022-09-16T11:09:18Z

R/modify.R

-    .x <- .x[rep(1L, length(out))]
+  if (vec_is_list(.x) || is.data.frame(.x)) {
+    out <- map2(vec_proxy(.x), .y, .f, ...)
+    vec_restore(out, .x)


Same here regarding df restoration.

lionel- · 2022-09-16T11:10:22Z

R/modify.R

+  if (vec_is_list(.x) || is.data.frame(.x)) {
+    out <- vec_proxy(.x)
+    out[.where] <- map(out[.where], .f, ...)
+    vec_restore(out, .x)


df-restoration

R/pmap.R

lionel- · 2022-09-16T11:14:52Z

R/modify.R

+    .x[] <- map(.x, .f, ...)
+    .x
+  } else {
+    cli::cli_abort("Don't know how to modify {.obj_type_friendly {.x}}.")


Can we use a "can't" form here?

! Can't modify a function.

Actually a "must" form works well I think:

! `.x` must be a vector, list, or data frame, not a function.

Do you think I should just ignore the fact it's a generic? It is unlikely that people have provided methods. Do you think I verify that there aren't methods on CRAN then remove the genericity?

hmm yes I think that is a good idea.

There are five packages with S3method(modify, in their namespace (https://cs.github.com/?scopeName=All+repos&scope=&q=org%3Acran+path%3A%2F%5ENAMESPACE%2F+%2FS3method%5C%28modify%2C%2F)

heemod — own modify generic

timbr — purrr generic

purrr — duh

tfarima — own generic

yamlet — own generic

timbr's modify.forest looks considerably more complex than our implementations, so it's probably ok for it to be its own function: https://github.com/UchidaMizuki/timbr/blob/main/R/purrr.R. Issue at UchidaMizuki/timbr#1

DavisVaughan · 2022-09-16T13:48:16Z

R/map.R

+#' * `_vec()` return an atomic or S3 vector, that is guaranteed to be
+#'   simpler than list.


that is guaranteed to be simpler than list.

Well, yes, but also: 😬

> map_vec(list(list(1:3), list(5)), identity) [[1]] [1] 1 2 3 [[2]] [1] 5

R/map2.R

R/modify.R

DavisVaughan · 2022-09-16T14:03:50Z

R/modify.R

+    vec_restore(out, .x)
+  } else if (vec_is(.x)) {
+    map_vec(.x, .f, ..., .ptype = .x)
+  } else if (is.null(.x) || is.list(.x)) {


FWIW I also prefer the explicit is.null(x) branch

R/modify.R

DavisVaughan · 2022-09-16T14:22:38Z

R/modify.R

+  if (vec_is_list(.x) || is.data.frame(.x)) {
+    out <- map2(vec_proxy(.x), .y, .f, ...)
+    vec_restore(out, .x)
+  } else if (vec_is(.x)) {


This is simultaneously technically correct and also makes me sad that we generate the duplicated column names

modify2(data.frame(x = 1), 1:3, \(x, y) x) #> x x x #> 1 1 1 1

tests/testthat/_snaps/map.md

DavisVaughan · 2022-09-16T14:28:12Z

tests/testthat/test-map2.R

-  )
+test_that("don't evaluate symbolic objects (#428)", {
+  map2(exprs(1 + 2), NA, ~ expect_identical(.x, quote(1 + 2)))
+  walk2(exprs(1 + 2), NA, ~ expect_identical(.x, quote(1 + 2)))
 })


Maybe a map2_vec() test about enforcing .ptype

DavisVaughan · 2022-09-16T14:30:53Z

tests/testthat/test-pmap.R

-  expect_identical(pmap_int(named_list, identity), named(int()))
-  expect_identical(pmap_dbl(named_list, identity), named(dbl()))
-  expect_identical(pmap_chr(named_list, identity), named(chr()))
+  pwalk(list(exprs(1 + 2)), ~ expect_identical(.x, quote(1 + 2)))
 })


Maybe a pmap_vec() test about enforcing .ptype (which would catch what Lionel pointed out before about missing the ptype arg)

And fix bug thus revealed

Conflicts: NEWS.md

Implement map_vec()

9a82a56

Fixes #435

hadley requested review from lionel- and DavisVaughan August 27, 2022 18:55

hadley mentioned this pull request Aug 27, 2022

Implement map_vec() #683

Closed

2 tasks

hadley changed the title ~~Implement map_vec()~~ Implement map_vec() Aug 27, 2022

lionel- reviewed Aug 29, 2022

View reviewed changes

R/map.R Outdated Show resolved Hide resolved

tests/testthat/test-map.R Show resolved Hide resolved

R/map.R Outdated Show resolved Hide resolved

DavisVaughan reviewed Aug 29, 2022

View reviewed changes

R/map.R Outdated Show resolved Hide resolved

hadley added 3 commits August 29, 2022 16:06

Implementation feedback

194828f

Merged origin/main into map-vec

a717b6c

Add tests to clarify behaviour

7733dc1

lionel- approved these changes Aug 30, 2022

View reviewed changes

DavisVaughan approved these changes Aug 30, 2022

View reviewed changes

R/map.R Outdated Show resolved Hide resolved

R/map.R Outdated Show resolved Hide resolved

R/map.R Outdated Show resolved Hide resolved

mgirlich mentioned this pull request Sep 5, 2022

More efficient pivot_longer() tidyverse/tidyr#1392

Merged

lionel- mentioned this pull request Sep 12, 2022

Basic map implementation r-lib/vctrs#495

Closed

hadley added 3 commits September 14, 2022 15:46

Merge commit '3b5add2db99a35ec1392ad23dc021b7ccadbbbbb'

bdde69f

Conflicts: R/map.R man/map.Rd tests/testthat/_snaps/map.md

Use new simplify tooling

63119fd

Remove unneeded import

0aff22d

hadley added 11 commits September 15, 2022 10:09

Merged origin/main into map-vec

b852864

Update snapshot

5671d9d

Update modify and modify_at

4d5b102

Add map2_vec() and pmap_vec()

ed04346

And add missing .progress argument to map_vec(). And move map_vec() to correct location.

Move map_chr to correct position

e093621

Update modify2

5167acc

And modify_if

08fe6e6

And extract out modify_where

Add news bullets

b2c09b3

Minimise map_vec() tests given implementation

903bdd3

Update map2 tests

e203d7e

Update pmap tests

3da5be6

hadley changed the title ~~Implement map_vec()~~ Apply vctrs principles to map() and modify() Sep 15, 2022

hadley added 2 commits September 15, 2022 16:55

At test for non-vector lists

8f82675

And fix bugs thus revealed

Test fallbacks

4983571

hadley requested review from lionel- and DavisVaughan September 15, 2022 22:00

lionel- approved these changes Sep 16, 2022

View reviewed changes

DavisVaughan approved these changes Sep 16, 2022

View reviewed changes

hadley added 7 commits September 16, 2022 09:55

Tweak modify's handling of data frames

961eeea

modify functions are no longer generics

2e7af8c

Polish modify tests

0b7ce2e

Tweak error message

ecae803

Add test for ptype

524e484

And fix bug thus revealed

Tweak return type description some more

9d45521

Merge commit '426acdd50424b8cd6029d237c4d4e81d94ec42a6'

87cd328

Conflicts: NEWS.md

hadley merged commit db7bd02 into main Sep 17, 2022

hadley deleted the map-vec branch September 17, 2022 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply vctrs principles to `map()` and `modify()` #894

Apply vctrs principles to `map()` and `modify()` #894

hadley commented Aug 27, 2022 •

edited

Loading

lionel- left a comment

hadley commented Aug 29, 2022

lionel- left a comment

hadley commented Sep 14, 2022

hadley commented Sep 15, 2022

lionel- Sep 16, 2022

hadley Sep 16, 2022

DavisVaughan Sep 16, 2022

hadley Sep 16, 2022

lionel- Sep 16, 2022

lionel- Sep 16, 2022

lionel- Sep 16, 2022 •

edited

Loading

hadley Sep 16, 2022

lionel- Sep 16, 2022

hadley Sep 16, 2022 •

edited

Loading

DavisVaughan Sep 16, 2022

DavisVaughan Sep 16, 2022

DavisVaughan Sep 16, 2022

DavisVaughan Sep 16, 2022

DavisVaughan Sep 16, 2022

		#' * `_vec()` return an atomic or S3 vector, that is guaranteed to be
		#' simpler than list.

Apply vctrs principles to map() and modify() #894

Apply vctrs principles to map() and modify() #894

Conversation

hadley commented Aug 27, 2022 • edited Loading

lionel- left a comment

Choose a reason for hiding this comment

hadley commented Aug 29, 2022

lionel- left a comment

Choose a reason for hiding this comment

hadley commented Sep 14, 2022

hadley commented Sep 15, 2022

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

lionel- Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley Sep 16, 2022 • 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

Apply vctrs principles to `map()` and `modify()` #894

Apply vctrs principles to `map()` and `modify()` #894

hadley commented Aug 27, 2022 •

edited

Loading

lionel- Sep 16, 2022 •

edited

Loading

hadley Sep 16, 2022 •

edited

Loading