fix: honor explicit zero valence instead of hallucinating hydrogens#230
fix: honor explicit zero valence instead of hallucinating hydrogens#230fbaensch-beilstein wants to merge 2 commits into
Conversation
An explicit zero valence (V3000 VAL=-1, normalized to the V2000 valence field value 15) was only special-cased in get_num_H when the atom had no bonds. A zero-valence atom carrying a bond fell into the literal-valence branch and got num_H = 15 - chem_bonds_valence implicit hydrogens, e.g. ethane with VAL=-1 on both carbons yielded InChI=1S/C2H28/c1-2/h1-2H14. Treat valence 15 as zero valence unconditionally: no implicit H regardless of whether bonds were drawn (InChI=1S/C2/c1-2 for the example above). Verified no output drift: baseline vs fixed libinchi produce identical InChIs across all 4190 records of both CI regression datasets. Closes #105
Unit Test Coverage ReportCoverage Regression Summary
Find details on the base coverage at https://iupac-inchi.github.io/InChI/coverage/index.html Find details on this PR's coverage by downloading coverage-reports-29074940986 and opening html/index.html |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect implicit-hydrogen assignment when an explicit zero valence is provided in molfile input (V3000 VAL=-1, normalized to V2000 valence-field 15). This aligns InChI output with the “honor zero valence” behavior discussed in issue #105 and prevents large, invalid “hallucinated hydrogen” counts for bonded atoms.
Changes:
- Update
get_num_Hto treat input valence15as zero valence unconditionally (no implicit H added even if bonds are present). - Add a regression test covering both V3000 (
VAL=-1) and V2000 (valence field15) encodings using a minimal ethane example.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| INCHI-1-TEST/tests/test_executable/test_github_105.py | Adds regression coverage for explicit zero-valence inputs to ensure no implicit H are added. |
| INCHI-1-SRC/INCHI_BASE/src/util.c | Fixes implicit-H calculation to honor explicit zero valence (15) regardless of drawn bonds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JanCBrammer
left a comment
There was a problem hiding this comment.
I can confirm that the test is red pre-fix and green post-fix (ran test locally). Good to merge as far as I'm concerned.
Summary
Fixes #105. An explicit zero valence —
VAL=-1in V3000, normalized to the V2000 valence-field value15— was only special-cased inget_num_Hwhen the atom had no bonds. A zero-valence atom carrying a bond fell into the literal-valence branch and receivednum_H = 15 - chem_bonds_valenceimplicit hydrogens.Concretely, ethane with
VAL=-1on both carbons produced:(14 "hallucinated" hydrogens per terminal carbon =
15 - 1).Fix
In
get_num_H(INCHI_BASE/src/util.c), treat valence15as zero valence unconditionally — no implicit H regardless of whether bonds were drawn. The isolated-atom behavior is unchanged; only the bonded case is corrected:Test
New
INCHI-1-TEST/tests/test_executable/test_github_105.py, parametrized over both V3000 (VAL=-1) and V2000 (valence field15), asserting the zero-valence ethane givesInChI=1S/C2/c1-2. Written test-first: confirmed RED on the old binary (C2H28), GREEN after the fix.Verification
test_executable): no unexpected failures.ctest): 15/15 pass.test_meta: pass.Arsine,phosphabicyclo, mcule1609394746) were confirmed pre-existing by rebuildinglibinchifrom baseline and reproducing them identically — not introduced here.libinchiproduce byte-identical InChIs across all 4190 records of both CI regression datasets. The new branch only fires for an explicit15/VAL=-1valence field, which real PubChem/mcule structures never carry.Note
This implements the "honor the zero valence" resolution from the issue discussion. The stricter "reject contradictory input (bond + zero valence) as an error" option is intentionally not taken here — it would be a larger, behavior-changing diff better handled separately.