tidymodels / dials

Tools for creating tuning parameter values
https://dials.tidymodels.org/
Other
111 stars 26 forks source link

Failure with dev scales #217

Closed hadley closed 2 years ago

hadley commented 2 years ago

Looks like maybe you've accidentally store some scales function in the package and it gets compared to the current version?

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure (test-extract_parameter_dials.R:4:3): regular usage ─────────────────
extract_parameter_dials(mod_param, "lambda") (`actual`) not equal to penalty() (`expected`).

body(actual$trans$breaks)[6:25] vs body(expected$trans$breaks)[6:28]
  `    rng <- log(raw_rng, base = base)`
  `    min <- floor(rng[1])`
  `    max <- ceiling(rng[2])`
- `    if (max == min) `
+ `    if (max == min) {`
  `        return(base^min)`
+ `    }`
  `    by <- floor((max - min)/n) + 1`
  `    breaks <- base^seq(min, max, by = by)`
  `    relevant_breaks <- base^rng[1] <= breaks & breaks <= base^rng[2]`
- `    if (sum(relevant_breaks) >= (n - 2)) `
+ `    if (sum(relevant_breaks) >= (n - 2)) {`
and 13 more ...

body(actual$trans$minor_breaks)[1:7] vs body(expected$trans$minor_breaks)[1:8]
  `{`
  `    b <- b[!is.na(b)]`
- `    if (length(b) < 2) `
+ `    if (length(b) < 2) {`
  `        return()`
+ `    }`
  `    bd <- diff(b)[1]`
  `    if (!reverse) {`
  `        if (min(limits) < min(b)) `

body(actual$trans$format)[1:6] vs body(expected$trans$format)[1:7]
  `{`
- `    if (!is.null(names(x))) `
+ `    if (!is.null(names(x))) {`
  `        return(names(x))`
+ `    }`
  `    ret <- format(x, ..., trim = TRUE, justify = "left")`
  `    ret[is.na(x)] <- NA`
  `    ret`
── Failure (test-extract_parameter_dials.R:8:3): regular usage ─────────────────
extract_parameter_dials(wflow_param, "lambda") (`actual`) not equal to penalty() (`expected`).

body(actual$trans$breaks)[6:25] vs body(expected$trans$breaks)[6:28]
  `    rng <- log(raw_rng, base = base)`
  `    min <- floor(rng[1])`
  `    max <- ceiling(rng[2])`
- `    if (max == min) `
+ `    if (max == min) {`
  `        return(base^min)`
+ `    }`
  `    by <- floor((max - min)/n) + 1`
  `    breaks <- base^seq(min, max, by = by)`
  `    relevant_breaks <- base^rng[1] <= breaks & breaks <= base^rng[2]`
- `    if (sum(relevant_breaks) >= (n - 2)) `
+ `    if (sum(relevant_breaks) >= (n - 2)) {`
and 13 more ...

body(actual$trans$minor_breaks)[1:7] vs body(expected$trans$minor_breaks)[1:8]
  `{`
  `    b <- b[!is.na(b)]`
- `    if (length(b) < 2) `
+ `    if (length(b) < 2) {`
  `        return()`
+ `    }`
  `    bd <- diff(b)[1]`
  `    if (!reverse) {`
  `        if (min(limits) < min(b)) `

body(actual$trans$format)[1:6] vs body(expected$trans$format)[1:7]
  `{`
- `    if (!is.null(names(x))) `
+ `    if (!is.null(names(x))) {`
  `        return(names(x))`
+ `    }`
  `    ret <- format(x, ..., trim = TRUE, justify = "left")`
  `    ret[is.na(x)] <- NA`
  `    ret`
hadley commented 2 years ago

The problem is:

load(test_path("data", "test_object.RData"))

test_that("regular usage", {
  expect_equal(extract_parameter_dials(mod_param, "lambda"), penalty(), ignore_function_env = TRUE)
  expect_equal(extract_parameter_dials(mod_param, "mixture"), mixture(c(0.05, 1)))
  expect_equal(extract_parameter_dials(rec_param, "wts"), spline_degree(c(1, 15)))
  expect_equal(extract_parameter_dials(rec_param, "disp"), spline_degree(c(1, 15)))
  expect_equal(extract_parameter_dials(wflow_param, "lambda"), penalty(), ignore_function_env = TRUE)
  expect_equal(extract_parameter_dials(wflow_param, "mixture"), mixture(c(0.05, 1)))
  expect_equal(extract_parameter_dials(wflow_param, "wts"), spline_degree(c(1, 15)))
  expect_equal(extract_parameter_dials(wflow_param, "disp"), spline_degree(c(1, 15)))
})

Since you're comparing a scales::trans object created with an old version and the new version of scales. This seems to be some sort of regression test, and I'm not sure what you're worried about regressing so I don't think I can fix it without further advice.

DavisVaughan commented 2 years ago

I think the reason we did this is because mod_param, rec_param, and wflow_param relied on parsnip, recipes, and workflows respectively to generate these objects. dials doesn't import any of these packages so we just created the objects and saved them.

At the end of the day, all 3 of these objects are just parameters objects from dials, they don't contain anything specific to those packages in them, so as long as we test dials:::extract_parameter_dials.parameters() in some other way with a parameters object created through dials, I don't think we need these tests (or the data objects created from here https://github.com/tidymodels/dials/tree/main/data-raw) at all? @hfrick does that sound right?

hfrick commented 2 years ago

yup, I've cleaned that up now 👍

hadley commented 2 years ago

Do you mind doing a patch release so I can submit scales to CRAN this week?

hfrick commented 2 years ago

sure, no problem!

github-actions[bot] commented 2 years ago

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.