Repository navigation
EEM Partial Charges for 3D descriptors - #376
Merged
Merged
Conversation
Contributor
 [Medium impact] The changes appear safe to merge; no new blocking issue was found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
EEMcharges reproduces RDKit's CalcEEMcharges for every conformer. It is the first property with one output row per atom: a new Property3DExtent says whether a property's rows are conformers or atoms, and per-atom outputs are laid out like the coordinate positions, indexed by the batch's atomStarts. Device3DPropertyResult now carries atom_starts (shared with Device3DResult input) and dense() pads per-atom properties to (n_mols, max_confs, max_atoms) with an atom_mask; the padding helper is shared with Device3DResult.dense(). One block per conformer builds RDKit's bordered EEM system and solves it by LU with partial pivoting, in shared memory when it fits and per-block global scratch otherwise. As in RDKit (which hands its row-major matrix to Eigen as column-major), charges sum to the negated formal charge. SINGLE refines the float32 solution once against a float64 residual: float32 elimination alone drifted up to ~3e-4 on 160-340 atom conformers, refinement brings SINGLE within 8e-7 of FULL. FULL matches RDKit to 1e-12. Atoms the parameter set does not cover produce NaN for their conformer: elements beyond bromine and bond orders above 3, where RDKit reads undefined memory, and, unlike RDKit, zero-parameter atoms within its tables (Si, single-bonded P, Na+ counter-ions, ...), which RDKit silently solves to charges of -5 to -168. Molecules that fail to kekulize and conformers with coincident atoms (where RDKit returns all-zero charges) are NaN as well.
RDKit before 2026.9.2 solves a transposed EEM system (rdkit/rdkit#9688), so its charges for charged molecules sum to the negated formal charge. Solve the published equations instead, so charges sum to the formal charge. Tests compare against RDKit only for neutral molecules on RDKit < 2026.9.2, and add golden-value cases for charged molecules that hold on every version.
…rmers, harden dense() - Conformers above 46,000 atoms give NaN instead of overflowing 32-bit indexes. - Size global scratch from the largest conformer actually solved, not the largest molecule in the batch. - Compare EEM charges in the benchmark only where the installed RDKit is correct. - dense() leaves invalid atom ranges NaN and builds per-atom gather indexes only when a per-atom property is requested.
scal444
force-pushed
the
feat/eem-charges-ready
branch
from
October 23, 2026 18:30
8ec23dc to
0d341ff
Compare
evasnow1992
reviewed
Oct 23, 2026
| # RDKit before 2026.9.2 solved the transposed EEM system (rdkit/rdkit#9688), so its charges for charged molecules | ||
| # sum to the negated formal charge. nvMolKit gives the correct values; only neutral molecules are compared against | ||
| # older RDKit, and the golden tests below cover the charged ones. | ||
| RDKIT_EEM_CHARGES_CORRECT = tuple(int(part) for part in rdkit.__version__.split(".")[:3]) >= (2026, 9, 2) |
Collaborator
There was a problem hiding this comment.
RDKit built from its development branch may report a version like 2026.09.1pre, where int("1pre") raises a ValueError. The benchmark script has the same issue.
Could we use re.findall(r"\d+", rdkit.__version__)[:3] to extract the version numbers instead?
evasnow1992
reviewed
Oct 23, 2026
| return; // Every row stays NaN. | ||
| } | ||
|
|
||
| const int numBlocks = std::min(coordinates.numConformers, numSms * 16); |
Collaborator
There was a problem hiding this comment.
The scratch memory allocation scales with both the largest conformer in the batch and the number of GPU blocks. This could lead to excessive memory usage or even OOM when a batch contains many conformers but only a few large ones. Could we consider limiting the scratch allocation based on a memory budget maybe?
evasnow1992
approved these changes
Oct 23, 2026
evasnow1992
left a comment
evasnow1992
left a comment
Collaborator
There was a problem hiding this comment.
Two comments, non-blocker. Changes look good to me.
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.
No description provided.