Diamond cond with OrConjunction #219
Conversation
Codecov Report
@@ 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.
|
eddiebergman
left a comment
There was a problem hiding this comment.
Just need to ask about the odd == thing. Otherwise a rough understanding of the code and the test seem to be correct.
|
|
||
| if active: | ||
| hps_to_be_activate.add(current_idx) | ||
| if current_value == current_value: |
There was a problem hiding this comment.
This seems odd? current_value == current_value, is this not always true?
There was a problem hiding this comment.
Not really, if current_value is deactivated, then this equation does not hold (Similar code can be found under line 335 and 344)
There was a problem hiding this comment.
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.
This PR works on solving a potential error that might occur for a hierarchy configuration space with OrConjunction:
Suppose that an HP
tophas two choicesAandBand an HPbottomthat is conditioned onAorB:(bottom | top == A || top == B). Then iftopswitches fromAtoB,bottomshould 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