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

Design check #270

Closed
wants to merge 2 commits into from
Closed

Design check #270

wants to merge 2 commits into from

Conversation

mb706
Copy link
Contributor

@mb706 mb706 commented Mar 14, 2020

Closes #268

This is a bit of an "opinionated" PR:

  • checking a df against a ParamSet could have been done with a new function (either member of ParamSet or global), but instead the $check() is polymorphic now. This gives us $assert and $test for free. Note I had to split up $check for the common stuff between data.frame and list to avoid unnecessary repeated checks of parameter names in the data.frame case
  • Design$new() now checks for feasibility of its argument, because implicitly users of this were assuming that it does (principle of least surprise)
  • Design$new() used to "enforce" dependencies; this is optional now and does not happen by default (see point above). This may break one or two users somewhere that relying on default enforcement of deps.
  • Design$new() parameter remove_dupl defaults to FALSE now because that is obviously the right choice for a default here.

If there are small problems with this please fix or delegate to a hiwicontributor.

@codecov-io
Copy link

Codecov Report

Merging #270 into master will decrease coverage by 0.08%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
- Coverage   95.16%   95.07%   -0.09%     
==========================================
  Files          20       20              
  Lines         496      508      +12     
==========================================
+ Hits          472      483      +11     
- Misses         24       25       +1
Impacted Files Coverage Δ
R/Sampler.R 87.5% <100%> (ø) ⬆️
R/generate_design_grid.R 91.3% <100%> (ø) ⬆️
R/generate_design_lhs.R 100% <100%> (ø) ⬆️
R/Design.R 94.59% <100%> (+0.15%) ⬆️
R/ParamSet.R 96.83% <96.87%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 745d89b...69ce0b5. Read the comment docs.

@berndbischl
Copy link
Sponsor Member

@mb706 @be-marc
apparently we should all communicate better.
marc already did the check_dt function.

i will keep the simpler solution and close this for now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

we need a function in ParamSet that checks whether a dt contains only feasible values
3 participants