Skip to content

feat: check for missing parameters - #422

Merged
be-marc merged 20 commits into
mainfrom
allow_subset
Nov 25, 2025
Merged

be-marc merged 20 commits into
mainfrom
allow_subset

Conversation

@be-marc

@be-marc be-marc commented Nov 20, 2025

Copy link
Copy Markdown
Member
param_set = ps(
  x1 = p_fct(levels = c("a", "b", "c")),
  x2 = p_dbl(lower = 0, upper = 1),
  x3 = p_int(lower = 0, upper = 1, depends = x1 %in% c("a", "b"))
)

param_set$check(list(x1 = "a", x2 = 1, x3 = 0), allow_subset = FALSE) # TRUE
param_set$check(list(x1 = "c", x2 = 1), allow_subset = FALSE) # TRUE
param_set$check(list(x1 = "b", x2 = 1), allow_subset = FALSE) # FALSE

Checks for missing parameters. Parameters are only allowed to be missing if their dependencies are unsatisfied.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a new allow_subset parameter to the ParamSet validation methods (check, test, assert, check_dt, test_dt, assert_dt) that enables checking whether all parameters are present in a configuration. When allow_subset = FALSE, all parameters must be present except for dependent parameters whose dependencies are not satisfied. This provides stricter validation for complete parameter configurations.

Key Changes:

  • Added allow_subset parameter (default TRUE for backward compatibility) to all validation methods
  • Implemented logic to verify all parameters are present when allow_subset = FALSE, with proper handling of dependencies
  • Added comprehensive test coverage for single and multi-parent dependency scenarios

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
R/ParamSet.R Implements allow_subset parameter logic in check(), test(), assert(), check_dt(), test_dt(), and assert_dt() methods with dependency satisfaction checking
tests/testthat/test_ParamSet.R Adds comprehensive test cases covering missing parameters, parent parameters, and dependent parameters with satisfied/unsatisfied dependencies
man/ParamSet.Rd Updates documentation for all modified methods to describe the new allow_subset parameter
NEWS.md Documents the new feature in the changelog
DESCRIPTION Updates RoxygenNote version and removes trailing whitespace
.lintr Updates linter configuration (unrelated housekeeping)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread man/ParamSet.Rd Outdated
Comment thread man/ParamSet.Rd Outdated
Comment thread man/ParamSet.Rd Outdated
Comment thread R/ParamSet.R Outdated
Comment thread R/ParamSet.R Outdated
Comment thread R/ParamSet.R Outdated
be-marc and others added 9 commits November 20, 2025 13:38
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Comment thread .lintr Outdated
Comment thread NEWS.md Outdated
Comment thread R/ParamSet.R Outdated
Comment thread R/ParamSet.R
#' If `"none"` (default), no check is performed for the presence of parameters.
#' If `"all"`, all parameters must be present in `xs`, except for parameters with unsatisfied dependencies.
#' If `"required"`, required parameters must be present in `xs`, except for parameters with unsatisfied dependencies.
#' For `"all"` and `"required"`, `TuneToken`s are not allowed to be present in `xs`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say (dis)allowing tune tokens is orthogonal to requiring presence, maybe we want to have an extra argument allow_tunetoken (default TRUE)? Also, do we want to forbid InternalTuneToken?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, for the sake of dependencies in $check_dependencies we are more liberal w/r/t TuneToken. The view there seems to be that if something is a TuneToken, the parameter is present for the sake of being depended on.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added allow_token = TRUE

be-marc and others added 6 commits November 24, 2025 10:18
Co-authored-by: mb706 <mb706@users.noreply.github.com>
Co-authored-by: mb706 <mb706@users.noreply.github.com>
@be-marc
be-marc merged commit 3730efb into main Nov 25, 2025
10 checks passed
@be-marc
be-marc deleted the allow_subset branch November 25, 2025 11:27
@be-marc
be-marc restored the allow_subset branch November 25, 2025 11:27
@be-marc
be-marc deleted the allow_subset branch November 25, 2025 11:32
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