Repository navigation
Write V3000 SDfile for more than 999 atoms - #294
Open
cm-beilstein wants to merge 12 commits into
Open
cm-beilstein wants to merge 12 commits into
cm-beilstein wants to merge 12 commits into
Conversation
OrigAtData_WriteToSDfile always writes V2000. Its counts fields are
3 chars wide, so > 999 atoms or bonds yields a broken counts line
("1000999 ..."). The test pins the V3000 output expected instead and
fails until a V3000 writer exists.
Refs #75
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
OrigAtData_WriteToSDfile always wrote V2000, whose counts fields are
3 chars wide; > 999 atoms or bonds produced a broken counts line
("1000999 ...") that no reader accepts. Such structures now get a
V3000 CTAB (atoms with CHG/RAD/MASS/VAL, bonds with CFG). Aliases
for isotopic implicit H still follow as V2000 "A aaa" properties,
which the V3000 reader already parses.
The alias reader took only 3 chars of the atom number, so "A 1000"
attached the alias to atom 100. It now reads the whole number.
Fixes #75
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
V3000 output must not carry V2000 property lines: isotopic implicit H (V2000 alias "A aaa") and polymer units (M STY/SAL/SBL/SDI) need V3000 equivalents inside the CTAB. Coordinates should keep V2000's 4 decimals. Also pin RAD= and CFG= output. Refs #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The V3000 output still appended V2000 property lines after END CTAB: "A aaa" aliases for isotopic implicit H and M STY/SAL/SBL/SDI for polymers. That is not valid V3000, and the 3-char alias atom field cannot address atoms > 999. Implicit D/T now become explicit H atoms with MASS= (D/T with AtomsDT) bonded to their heavy atom, so the InChI round trip is unchanged. Polymer units become a V3000 SGROUP block (ATOMS, XBONDS, BRKXYZ, CONNECT, SUBTYPE, LABEL), as read by the GH #146 reader; long lines wrap with "-". Coordinates use 4 decimals, as in V2000. The alias reader change is reverted: V2000 atom numbers never exceed 999, so it fixed no V2000 input. Refs #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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-41029753860 and opening html/index.html |
Pins MASS= as an absolute mass, as V2000 M ISO writes it. It passes already: element-symbol and element-number masses agree for every reachable atom, including pseudoatoms (both 0). Refs #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MASS used the mass of the written symbol, which is "*" for Zz/Zy pseudoatoms. Use the element-number mass like V2000 M ISO instead, so both writers cannot drift apart. No reachable output changes: pseudoatom masses are 0 either way. Refs #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Implicit D/T are written as explicit atoms at their heavy atom's coordinates. In 2D that is a zero-length bond: at a CHD stereocentre the InChI read back loses its /t layer. The test compares against the same structure with D drawn explicitly, and checks that 0D input keeps the D at the origin. Refs #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Implicit D/T written as explicit atoms sat on their heavy atom: a zero-length bond that drops the stereo of a 2D CHD stereocentre. Place them at the average bond length, opposite the neighbours' centroid, as strutil.c moves an H; further ones turn by 30 degrees. 0D input (all bonds zero length) keeps the D/T at the heavy atom. Refs #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the neighbours' centroid coincides with the atom (here 60, 180 and 300 deg), IsoHOffset falls back to 180 deg, on top of an existing bond: the wedged stereocentre loses its /t layer on read-back. Refs #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If the neighbours' centroid coincides with the atom, the fallback direction of 180 deg could lie on an existing bond (straight chain, or neighbours at 60/180/300 deg), losing or flipping wedge stereo. Bisect the gap after neighbour 0 instead: perpendicular for two neighbours, between neighbours for three. Placement uses x/y only; 3D input is deferred. Refs #75 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved defects can produce degenerate stereochemistry, incorrect isotope-labelled polymer membership, overlong V3000 lines, and bondless output that the repository’s reader cannot parse.
4 open issues
What changed in this PR
Addresses #75 by switching SDfile output to V3000 when atom or bond counts exceed 999, while retaining V2000 for smaller structures.
Changes:
- Adds V3000 serialization for atom properties, bond stereochemistry, explicit isotopic hydrogens, and polymer Sgroups.
- Adds coordinate placement for generated D/T atoms to preserve 2D stereochemistry.
- Adds regression tests for count thresholds, isotope round trips, radicals, wedges, and polymer output.
| File | Description |
|---|---|
INCHI-1-TEST/tests/test_unit/test_github_75.cpp |
Tests format selection and V3000 chemical-property preservation. |
INCHI-1-SRC/INCHI_BASE/src/mol_fmt4.c |
Implements V3000 output and automatic selection above V2000 count limits. |
Review effort: Balanced
Comment on lines
+769
to
+770
| inchi_ios_print_nodisplay(fcb, "%s", token); | ||
| *col += len; |
| col = V3000_PREFIX_LEN - 1; | ||
| sprintf(token, " %d %s %d", u->id, sty[u->type], u->label); | ||
| V3000Put(fcb, &col, token); | ||
| V3000PutList(fcb, &col, "ATOMS", u->na, u->alist); |
Comment on lines
+926
to
+928
| f = atan2(at[i].y - cy, at[i].x - cx); | ||
| } | ||
| f += step * ((n + 1) / 2) * (n % 2 ? 1 : -1); |
| inchi_ios_print_nodisplay(fcb, "M V30 END ATOM\n"); | ||
|
|
||
| /* Bonds; e.g. "M V30 1 1 1 2 CFG=1" */ | ||
| inchi_ios_print_nodisplay(fcb, "M V30 BEGIN BOND\n"); |
…placement - Omit BEGIN/END BOND when there are no bonds: MolfileV3000ReadBondsBlock skips the block for a zero count, so the empty block made the CTAB tail reader fail on InChI's own output. - List the explicit D/T atoms in the polymer units of their parent atom, as the V2000 alias form keeps them inside the bracket. - Cut a token longer than a line over "-" continuation lines instead of writing a line over 80 characters. - Place explicit D/T by InChI's own implicit-H rule (opposite the sum of the neighbour unit vectors, wedges lifted) and bisect the neighbour gap that contains it, checking the 2D parity for 3-neighbour centres. The old centroid rule could put the D/T on a bond (e.g. neighbours at 0, 90, 225 deg) and, for a wedged symmetric centre, on the side that inverts the configuration InChI reads for an implicit H. Tests: bondless molecule round trip, D inside an SRU, 79-char label, off-centroid and symmetric centres, two D on one atom; the symmetric stereo test now takes InChI's implicit-H parity as reference. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Collaborator
Author
Fixed in f3e5428. The
Fixed in f3e5428. The explicit D/T atoms are now listed in the
Fixed in f3e5428. A token that does not fit is now cut and continued with a bare
Fixed in f3e5428. The D/T is now placed by the same rule InChI uses for an implicit H (opposite the sum of the neighbour unit vectors, wedges lifted), put into the middle of the neighbour gap that contains that direction, and for 3-neighbour centres the parity is checked against that reading. This also fixes a case where the old placement inverted the configuration at a wedged symmetric centre. Added tests for:
|
This branch has not been deployed
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
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.
Fixes #75.
-OutputSDFwrote a broken V2000 counts line (e.g.9891014) when atoms or bonds exceeded 999, the V2000 field width. Above that limit the output is now V3000; smaller structures stay V2000, unchanged.The V3000 output (
mol_fmt4.c) has onlyM V30lines between the counts line andM END:CHG/RAD/MASS/VAL; bonds withCFGfor wedges.Aalias) become explicit D/T atoms bonded to their heavy atom; V3000 has no inline alias.BEGIN SGROUPblock (ATOMS, XBONDS, BRKXYZ, CONNECT, SUBTYPE, LABEL).%.4f, as in V2000.Tests (
test_github_75): small molecule stays V2000; >999 atoms gives V3000; deuterium as V3000 atom with round trip to/i1D; radical and wedge; polymer Sgroup.Notes: