r-lib / pillar

Format columns with colour
https://pillar.r-lib.org/
Other
178 stars 37 forks source link

feat: Math operations on `num()` objects now pass additional arguments to the mathematical function #660

Closed gvelasq closed 5 months ago

gvelasq commented 5 months ago

Fixes #659

Thank you for considering a pull request. As I looked into this, I noticed that sum() and other functions that take additional arguments (e.g., mean(), prod()) are handled by vec_math_base() instead of vec_arith_base(). I passed the ellipsis to vec_math_base() which now performs the correct calculation, and I added a test which passes.

# current behavior
library(pillar)
sum(num(c(1:3, NA)), na.rm = TRUE)
#> <pillar_num[1]>
#> [1] NA

# proposed behavior
devtools::load_all("~/rrr/pillar", quiet = TRUE)
sum(num(c(1:3, NA)), na.rm = TRUE)
#> <pillar_num[1]>
#> [1] 6

Created on 2024-06-16 with reprex v2.1.0

gvelasq commented 5 months ago

I reverted NEWS.md because now I see that this file is managed by fledge.

gvelasq commented 5 months ago

Thanks. Why do the tests fail?

I believe that the tests failed because passing the dots to vec_math_base() fixed a bug in this test snapshot. It's easier to reason about the log transformation using the test dataset's y variable on the y-axis, rather than the z variable on the y-axis as in the snapshot, so I am showing both examples in the reprex below. I compared the current and proposed behavior of pillar::scale_y_num(transform = "log10") to ggplot2::scale_y_continuous(transform = "log10").

One minor point is that the trans argument to ggplot2::continuous_scale() has been deprecated in favor of transform. I fixed this in the most recent commit to the pull request.

library(ggplot2)
library(pillar)

# load current scale_y_num()
library(tibble)

# data with num() objects used in test/testthat/test-ggplot2.R
df1 <-
  tibble(
    x = num((1:10) / 100, fixed_exponent = -3, notation = "eng"),
    y = num((1:10) / 100, scale = 100, label = "%"),
    z = num(10^(-5:4), notation = "si")
  )

# plain data
df2 <-
  tibble(
    x = seq(10, 100, by = 10),
    y = 1:10,
    z = 10^(-5:4)
  )

# current scale_y_num() on num() objects, x-y
p1xy <-
  ggplot(df1, aes(x, y)) +
  geom_point() +
  scale_y_num(transform = "log10") +
  theme_minimal(36)
p1xy
#> Warning: The `scale_name` argument of `continuous_scale()` is deprecated as of ggplot2
#> 3.5.0.
#> This warning is displayed once every 8 hours.
#> Call `lifecycle::last_lifecycle_warnings()` to see where this warning was
#> generated.


# current scale_y_num() on num() objects, x-z
p1xz <-
  ggplot(df1, aes(x, z)) +
  geom_point() +
  scale_y_num(transform = "log10") +
  theme_minimal(36)
p1xz


# current scale_y_continuous() on plain data, x-y
p2xy <-
  ggplot(df2, aes(x, y)) +
  geom_point() +
  scale_y_continuous(transform = "log10") +
  theme_minimal(36)
p2xy


# current scale_y_continuous() on plain data, x-z
p2xz <-
  ggplot(df2, aes(x, z)) +
  geom_point() +
  scale_y_continuous(transform = "log10") +
  theme_minimal(36)
p2xz


# load proposed scale_y_num()
devtools::load_all("~/rrr/pillar")
#> ℹ Loading pillar

# proposed scale_y_num() on num() objects, x-y
p3xy <-
  ggplot(df1, aes(x, y)) +
  geom_point() +
  scale_y_num(transform = "log10") +
  theme_minimal(36)
p3xy


# proposed scale_y_num() on num() objects, x-z
p3xz <-
  ggplot(df1, aes(x, z)) +
  geom_point() +
  scale_y_num(transform = "log10") +
  theme_minimal(36)
p3xz

Created on 2024-06-21 with reprex v2.1.0

krlmlr commented 5 months ago

Thanks!