fix: reject impossible CALYPSO atom choices - #382
Conversation
dosubot
Bot
added
size:L
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Correct the empty-set validation so nested atom choices that cannot produce unique species fail before the random selection loop. Closes deepmodeling#356 Coding-Agent: Codex Codex-Version: codex-cli 0.149.1 Model: gpt-5.6-sol Reasoning-Effort: xhigh
5507103 to
d3ca156
Compare
dosubot
Bot
added
size:XS
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #382 +/- ##
==========================================
+ Coverage 84.43% 84.45% +0.01%
==========================================
Files 104 104
Lines 6110 6110
==========================================
+ Hits 5159 5160 +1
+ Misses 951 950 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
You are right that (set(s) - overlap) == 0 is always False and the guard has been dead since it was written. But the guard's predicate was never correct either, and turning it on as-is rejects configurations that work on master today, while still leaving the infinite loop in place for the cases it misses.
The predicate is the wrong test
The loop below needs one distinct element per sub-list. That is exactly a system of distinct representatives, so the right condition is Hall's marriage theorem: for every subset of sub-lists, the union of their candidates must be at least as large as the subset. "Some sub-list is a subset of the global intersection" is neither sufficient nor necessary for that.
Brute-forcing every nested config over a 3-symbol universe (343 families, 247 satisfiable, 96 impossible):
master: wrongly rejects 0/247 satisfiable catches 0/96 impossible (dead code)
this PR: wrongly rejects 82/247 (33%) catches 51/96, misses 45
False positives - these all succeed on master and raise ValueError at d3ca156, verified through the real make_calypso_task_group_from_config:
| config | satisfiable as |
|---|---|
[["Li"]] |
Li |
[["Li","Na"]] |
either |
[["Li","Na"],["Li","Na"]] |
Li, Na |
[["Li","Na","K"],["Na","K"],["K"]] |
Li, Na, K |
The last row is worth pausing on: it is structurally the same as [[A,B,C], [B,C], [C]], the example the error message itself cites as "not allowed". That example is satisfiable - pick C for the third slot, B for the second, A for the first. So the message documents the wrong rule, and this PR now enforces it.
False negatives - [["Li","Na"],["Li","Na"],["Li","Na"],["K"]] has three slots drawing from two symbols, so no distinct assignment exists. The disjoint fourth sub-list drives the global intersection to empty, so the guard never fires and it still hangs at PR head. #356 asks to "avoid infinite loops"; roughly half of genuinely impossible configs still loop.
Two ways forward
Either works for me:
- Test Hall's condition.
numb_of_speciesis tiny, so iteratingcombinationsof the sub-lists and raising whenlen(set().union(*sub)) < len(sub)is cheap, sound and complete. Fix the error message and its example at the same time. - Replace the guard and the loop together with a matching-based pick - shuffle each sub-list's candidates and run an augmenting-path assignment. That is exact, always terminates, preserves the randomness the feature exists for, and makes the separate guard unnecessary.
If neither is appealing right now, the minimum I would want is a retry cap on the while True so an unsatisfiable config fails fast instead of spinning - but note that alone does not address the false positives above.
Why this has been invisible
The guard was born broken in 07df321 (#217, 2024-04-30) - there was never a len(...) == 0 that a cleanup dropped, and the loop never had a cap. tests/exploration/test_exploration_group.py does exercise the nested form via config_random, but only with configs whose global intersection is empty, so the guard's true branch has never been reached by any test. That is the cell this PR fills, and it is why a wrong predicate could sit there for two years.
One more thing worth knowing about the loop: the spin happens at the logging.info call inside it, so pre-fix this is a hot loop writing log lines at full CPU, running client-side inside dpgen2 submit before wf.submit(). Nothing reclaims it.
Separately and not for this PR: numb_of_species is never validated against len(name_of_atoms). A mismatch currently dies with a bare IndexError from the distance_of_ions branch rather than a clear message.
| overlap = overlap & set(temp) | ||
|
|
||
| if any(map(lambda s: (set(s) - overlap) == 0, name_of_atoms)): | ||
| if any(not (set(atom_choices) - overlap) for atom_choices in name_of_atoms): |
There was a problem hiding this comment.
The truthiness fix is right - the old == 0 compared a set to an int and never fired. But the condition being tested is not the one the loop below needs.
The loop requires a distinct element per sub-list, i.e. a system of distinct representatives, so the correct test is Hall's condition. "Some sub-list is a subset of the global intersection" is neither sufficient nor necessary. Over all 343 nested configs on a 3-symbol universe this predicate wrongly rejects 82 of 247 satisfiable ones and misses 45 of 96 impossible ones.
Concretely, [["Li"]], [["Li","Na"]] and [["Li","Na","K"],["Na","K"],["K"]] all work on master and raise here - the last being the same shape as the [[A,B,C],[B,C],[C]] in the message below, which is satisfiable as A,B,C.
Since the sub-lists are tiny, any(len(set().union(*sub)) < len(sub) for k in range(1, n+1) for sub in combinations(name_of_atoms, k)) is sound, complete and cheap. Please also correct the example in the error string - it currently documents a valid config as forbidden.
| def test_rejects_impossible_random_atom_choices(self): | ||
| """Fail before random selection when unique choices are impossible.""" | ||
| config = { | ||
| "name_of_atoms": [["Li"], ["Li"]], |
There was a problem hiding this comment.
[["Li"],["Li"]] is the one input where the wrong predicate and the correct one agree, so this passes for the right reason by coincidence and gives no signal about whether the rule generalises. That is how a 33% false-positive rate ships with green CI.
Two cases would pin the behaviour properly:
- a satisfiable nested config that must be accepted, e.g.
[["Li","Na","K"],["Na","K"],["K"]]; - a genuinely impossible config the current guard misses, e.g.
[["Li","Na"],["Li","Na"],["Li","Na"],["K"]], which must raise rather than hang.
Worth noting in the docstring that this is a liveness test: with the fix reverted it does not fail, it hangs - I had to kill it with a timeout. Without a per-test timeout, a regression here wedges the whole CI job rather than reporting one failure.
2 participants
Summary
ValueErrorbefore entering the random selection loop[["Li"], ["Li"]]Tests
PYTHONPATH=tests python -m unittest -v tests.exploration.test_make_task_group_from_config.TestMakeCalyTaskGroupFromConfig.test_rejects_impossible_random_atom_choices tests.exploration.test_make_task_group_from_config.TestMakeCalyTaskGroupFromConfig.test_caly_task_groupruff format --check dpgen2/exploration/task/caly_task_group.py tests/exploration/test_make_task_group_from_config.pyisort --check-only dpgen2/exploration/task/caly_task_group.py tests/exploration/test_make_task_group_from_config.pygit diff --checkCloses #356
Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh