Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

some more compatibility with new paradox version #991

Merged
merged 5 commits into from
Feb 29, 2024
Merged

Conversation

mb706
Copy link
Collaborator

@mb706 mb706 commented Jan 13, 2024

No description provided.

@mb706
Copy link
Collaborator Author

mb706 commented Jan 13, 2024

@mllg
this fails with the new paradox version in this test:

design = data.table(task = tasks, learner = learners, resampling = resamplings, param_values = list(list(list(cp = 0.1), list(minbucket = 2))))
bmr = benchmark(design)
expect_equal(bmr$learners$learner[[1]]$param_set$values, list(xval = 0, minsplit = 12, minbucket = 2))
expect_equal(bmr$learners$learner[[2]]$param_set$values, list(xval = 0, minsplit = 12, cp = 0.1))

However, I don't get it: the param_values argument in the design is list(cp = 0.1) for the first learner, yet the test expects cp = 0.1 in the second row of the result. Why is that?

@mb706
Copy link
Collaborator Author

mb706 commented Jan 13, 2024

is the row order in the benchmark result just coincidental and depends on the hash of objects? in this case, I would adapt this specific test to be row order independent.

@mb706 mb706 marked this pull request as draft January 14, 2024 11:13
@mb706 mb706 mentioned this pull request Jan 14, 2024
@mllg
Copy link
Member

mllg commented Jan 29, 2024

Yes, this is a bad test; you should reorder the names (as you already did in this draft) and then not rely on the order, e.g.:

  trained = bmr$learners$learner
  ii = which(map_lgl(trained, function(x) "cp" %in% names(x$param_set$values))) # find learner with cp
  expect_count(ii)

  expect_equal(trained[[ii]]$param_set$values, list(xval = 0, minsplit = 12, cp = 0.1))
  expect_equal(trained[[-ii]]$param_set$values, list(xval = 0, minsplit = 12, minbucket = 2))

@mb706 mb706 marked this pull request as ready for review February 28, 2024 14:37
@mb706
Copy link
Collaborator Author

mb706 commented Feb 28, 2024

@mllg this one can be merged

@be-marc be-marc merged commit 14905fb into main Feb 29, 2024
4 checks passed
@be-marc be-marc deleted the s3params_compat branch February 29, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants