Update molecular inorganics bond handling (GHI # 218)#219
Open
nnuk wants to merge 12 commits into
Open
Conversation
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-27753342980 and opening html/index.html |
cthoyt
reviewed
Jun 4, 2026
|
@JanCBrammer this needs your review |
|
There's a blocker on this related to a lithium hydroxide molecule that needs discussion (how covalent bond is represented) between @gblanke02 and @nnuk Nauman has a specific structure in mind. Not sure if we have the expected result. |
cthoyt
reviewed
Jun 9, 2026
cthoyt
reviewed
Jun 9, 2026
cthoyt
reviewed
Jun 9, 2026
Contributor
|
He InChI internal bond types will be described by a section about the internal object that Christoph works on.
In terms of bond types only the first 4 bond types correspond directly with the molfile definitions, bond types >= 4 are used to describe the bond environment of an atoms, i.e. the types of the bonds leaving from an atom are listed in atom specific arrays. More will be found in the documentation
Cheers,
Gerd
|
Contributor
|
The variable names are already pre-defined in the (legacy) code. For the time being we will not start a major re-factoring to come up to more descriptive names. Beside that, the meaning of “bond type” is pretty well understood in the cheminformatics area. See the Biovia CTFile document for that.
By the way, the bond type 9 means coordinative bonds in the CT File format but the meaning within InChI is different.
|
…rges Realize each type-9 (coordinative) bond as a single bond at the start of MolecularInorganicsPreprocessing, and cancel the paired +/- formal charges of the two bonded atoms (the donated lone pair becomes the shared bonding pair). This unifies the two equivalent coordinative-bond drawings - donor VAL set, or explicit +/- charges - so both yield the same InChI (e.g. NaOH drawn either way -> Na+ + OH-). Fixes the previously failing charged-input cases in test_molecularInorganics (now 11/11).
cthoyt
reviewed
Jun 15, 2026
JanCBrammer
reviewed
Jun 15, 2026
Collaborator
Author
|
next steps:
|
In MolecularInorganics mode, a metal and ligand carrying opposite formal charges across a single (type 1) or coordinative (type 9) bond depict an ionic interaction (e.g. M(2+) ... L(2-)) and must split into the drawn ions. Two heuristics in the disconnection loop prevented this: the per-element valence gate (which rejects e.g. Na+ bearing a bond) and the "keep all bonds when several metals share a component" rule (which kept Na-O-Na connected). Add a dedicated pass in MolecularInorganicsPreprocessing that disconnects such bonds up front via DisconnectInpAtBond, which also adapts valence and chem_bonds_valence. No charge is added or removed: each ion keeps exactly the charge drawn in the input. Higher-order bonds (e.g. a drawn M=O double bond) are genuine covalent bonds and are left intact. ConvertCoordinativeBondsToSingle is restricted back to type-9 bonds only; the charge cancellation that previously fired for plain charge-separated single bonds is removed, since those charges are intrinsic to the single-bond depiction. Standard InChI is unaffected (the code runs only under -MolecularInorganics). Adds unit tests for the singly/multiply-charged single and coordinative metal-ligand cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request resolves #218. Coordinative bond included in the condition to be evaluated before the disconnection of bonds.