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

Diamond cond with OrConjunction #219

Merged
merged 6 commits into from
Jun 8, 2022
Merged

Conversation

dengdifan
Copy link
Contributor

This PR works on solving a potential error that might occur for a hierarchy configuration space with OrConjunction:
Suppose that an HP top has two choices A and B and an HP bottom that is conditioned on A or B: (bottom | top == A || top == B). Then if top switches from A to B, bottom should remain to activate (A more concrete example could be found under test/test_util.py::UtilTest::test_check_neighbouring_config_diamond_or_conjunction).

An additional check is attached under change_hp_value. However, this might make the check slower: we need to check all the children conditions and not skip them as before

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #219 (3f62ff2) into master (4d69931) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #219   +/-   ##
=======================================
  Coverage   67.15%   67.15%           
=======================================
  Files          17       17           
  Lines        1629     1629           
=======================================
  Hits         1094     1094           
  Misses        535      535           

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 4d69931...3f62ff2. Read the comment docs.

Copy link
Contributor

@eddiebergman eddiebergman left a comment

Choose a reason for hiding this comment

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

Just need to ask about the odd == thing. Otherwise a rough understanding of the code and the test seem to be correct.

@@ -315,6 +322,16 @@ cpdef np.ndarray change_hp_value(
active = False
break

if active:
hps_to_be_activate.add(current_idx)
if current_value == current_value:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems odd? current_value == current_value, is this not always true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, if current_value is deactivated, then this equation does not hold (Similar code can be found under line 335 and 344)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, two NaNs compare as unequal, see https://en.wikipedia.org/wiki/NaN#Comparison_with_NaN. This is the fastest way to check for NaN values because it does not require any additional function calls.

@mfeurer mfeurer merged commit 3036164 into automl:master Jun 8, 2022
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