FIX #17/#26

[mfeurer]

Supersedes #31

Fixes #17 and #26 (together with the latest version of the config space).

[mfeurer]

@mlindauer @KEggensperger please review and comment on this. @mlindauer if you agree on this, can you please close #31?

[coveralls]

Coverage decreased (-0.05%) to 62.523% when pulling 1b9b1e8 on fix/#17/#26 into efe9cc0 on development.

[coveralls]

Coverage decreased (-0.05%) to 62.523% when pulling 1b9b1e8 on fix/#17/#26 into efe9cc0 on development.

[mlindauer]

Hi,

Thanks for the fix. It is working with my examples and the code should be fine.

However, I would like to propose a further change (because of efficiency and consistency).
The method Runhistory.get_runs_for_config() should get the config_id of config and compare the ids in the if-statement.
Comparison of int should be more efficient and we only need one dictionary access (instead of many in Line 111). Furthermore, it is more consistent to use also here the config_id as we do it also in objective._cost()

Cheers,
Marius

[mfeurer]

Would it be okay to add this as an issue and solve in a separate PR?

[mlindauer]

Would it be okay to add this as an issue and solve in a separate PR?

Yes.
I thought it is very related to your fix and it reduces the overhead to create again another PR with an additional review phase.

[mlindauer]

Ok, I will submit a PR for #35, after this PR was merged.

There is no "approve" button in Github?

Cheers,
Marius

[mfeurer]

Correct, there is no "approve" button. We could adapt the scikit-learn policy of changing the PR names like this:

• [WIP] name: pull request is work in progress
• name: pull request is ready to be reviewed
• [MRG] name: pull request is approved by one reviewer
• [MRG+2] name: pull request is approved by two reviewers.

[KEggensperger]

approved