Conversation
Codecov Report
@@ Coverage Diff @@
## devel #294 +/- ##
==========================================
- Coverage 81.57% 81.53% -0.04%
==========================================
Files 62 62
Lines 5244 5249 +5
==========================================
+ Hits 4278 4280 +2
- Misses 966 969 +3
Continue to review full report at Codecov.
|
wanghan-iapcm
left a comment
There was a problem hiding this comment.
This is a breaking change to the code that has potential influence on other codes using dpdata.MultiSystems with explicit formula.
I do not support the merging of this PR.
Actually this change just modifies the system names in the MultiSystems frames (add "_virials" to the system names), I do not see what negative influence this change will have on other codes. |
njzjz-bot
left a comment
There was a problem hiding this comment.
Requesting changes because the proposed virial suffix is not stable under MultiSystems atom-name normalization and is also a breaking change to public formula keys. The inline reproduction shows that the original mixed-label failure returns after appending a new element. This needs a stable label-schema grouping design that preserves the formula API, not a temporary key suffix.
Process note: the Codex usage allowance is about to reset, so I am spending the remaining token budget now on this review.
Coding agent: Codex
Codex version: codex-cli 0.144.6
Model: gpt-5.6-sol
Reasoning effort: xhigh
| self.check_atom_names(system) | ||
| formula = system.formula | ||
| if 'virials' in system.data: | ||
| formula = formula + "_virials" |
There was a problem hiding this comment.
[P1] Do not encode label schema in a temporary formula key
check_atom_names can rebuild the systems dictionary from each System.formula whenever a new element is introduced, so this suffix is lost. I reproduced keys changing from H2_virials to H2O0 after appending an O system; appending an unlabeled H2 then collides again and raises the original virial mismatch. The suffix also changes every public key and output directory for virial-bearing systems. Group by a stable field signature while preserving chemical formula keys instead.
njzjz-bot
left a comment
There was a problem hiding this comment.
The current head still has the previously identified correctness/backward-compatibility blocker. Encoding the label schema by appending _virials to the temporary formula key is not stable under MultiSystems.check_atom_names(), which can rebuild the systems mapping from each underlying System.formula when a new element is introduced. That drops the suffix, so a later append can collide virial-bearing and non-virial systems and reproduce the original mismatch. It also changes public MultiSystems.systems keys/output directory names for every virial-bearing system. The existing unresolved inline thread contains the concrete reproduction and exact location, so I am not duplicating it.
This needs a stable way to distinguish label schemas while preserving the chemical-formula API, with regression coverage that adds a new element and then appends a same-composition system with the opposite virial-label state. The branch is currently not mergeable against master, and there are no exact-head workflow runs for this SHA, so it will also need a rebase and fresh CI after the design is corrected.
Agent: ChatGPT
Model: GPT-5.6 Sol
GitHub account: njzjz-bot
Reviewed head: 9f7b9d4
Trigger: scheduled all-PR monitoring
Sometimes we came across with multisystem (such as quip/gap/xyz format files) which has both frames with virials labels and frames without virials labels. The current implementation of the Multisystems class will lead to error message:
But this kind of Multisystems are in fact trainable, so I identify frames with virials in Multisystems, such that Systems will have names as:
The former represents systems with 60 carbon atoms without virials labels, the latter represents systems with 60 carbon atoms with virials labels.