One liner descriptors added#554
Open
pkacprzak5 wants to merge 9 commits intomordredfrom
Open
Conversation
pkacprzak5
requested review from
j-adamczyk,
mjste and
my-alaska
as code owners
pkacprzak5
force-pushed
the
pk-rdkit-descriptors
branch
from
97be849 to
a02cf3f
Compare
j-adamczyk
requested changes
Apr 29, 2026
| @@ -0,0 +1,59 @@ | |||
| """Atom count descriptors implemented with direct RDKit atom access. | |||
Member
There was a problem hiding this comment.
We always start docstrings from new line
Comment on lines
+12
to
+25
| FEATURE_NAMES = [ | ||
| "nH", | ||
| "nB", | ||
| "nC", | ||
| "nN", | ||
| "nO", | ||
| "nS", | ||
| "nP", | ||
| "nF", | ||
| "nCl", | ||
| "nBr", | ||
| "nI", | ||
| "nX", | ||
| ] |
Comment on lines
+53
to
+56
| values.extend( | ||
| sum(atomic_num == _ELEMENTS[name] for atomic_num in atomic_nums) | ||
| for name in FEATURE_NAMES[1:-1] | ||
| ) |
Member
There was a problem hiding this comment.
This is inefficient. Just make a list of atomic numbers above (with comments for elements) and iterate over it here.
Comment on lines
+46
to
+48
| Hydrogen counts use RDKit's total hydrogen count on each atom, so implicit | ||
| hydrogens are included without adding explicit hydrogen atoms to the | ||
| molecule. Heavy-element counts are taken from the molecule's atom list. |
Member
There was a problem hiding this comment.
Technical details, unnecessary in a docstring
|
|
||
| def calc(mol: Mol) -> tuple[np.ndarray, list[str]]: | ||
| """ | ||
| Compute the remaining Mordred atom count descriptors. |
Member
There was a problem hiding this comment.
Why "remaining"? Those are just atom counts, and we should have all general atom counts in a single module
Comment on lines
+79
to
+83
| def _default_int_dict() -> defaultdict[int, int]: | ||
| """ | ||
| Create a nested default dictionary for carbon type counts. | ||
| """ | ||
| return defaultdict(int) |
Member
There was a problem hiding this comment.
Don't call a function like that (overhead), just use defaultdict(int) where necessary
Comment on lines
+26
to
+30
| "nAtom", | ||
| "nHeavyAtom", | ||
| "nSpiro", | ||
| "nBridgehead", | ||
| "nHetero", |
Member
There was a problem hiding this comment.
This looks perfect for atom_count.py
| "nSpiro", | ||
| "nBridgehead", | ||
| "nHetero", | ||
| "FCSP3", |
Member
There was a problem hiding this comment.
returns the fraction of C atoms that are SP3 hybridized - you calculate this in carbon_types.py anyway
Member
There was a problem hiding this comment.
I'm not sure which one will be faster, though, check that
Comment on lines
+149
to
+164
| _safe_value( | ||
| rdMolDescriptors.CalcNumRings, | ||
| mol_regular, | ||
| ), | ||
| _safe_value( | ||
| rdMolDescriptors.CalcNumHeterocycles, | ||
| mol_regular, | ||
| ), | ||
| _safe_value( | ||
| rdMolDescriptors.CalcNumAromaticRings, | ||
| mol_regular, | ||
| ), | ||
| _safe_value(rdMolDescriptors.CalcNumAromaticHeterocycles, mol_regular), | ||
| _safe_value(rdMolDescriptors.CalcNumAliphaticRings, mol_regular), | ||
| _safe_value(rdMolDescriptors.CalcNumAliphaticHeterocycles, mol_regular), | ||
| _safe_value(rdMolDescriptors.CalcNumRotatableBonds, mol_regular), |
Member
There was a problem hiding this comment.
You calculate this in ring_count.py, right?
Comment on lines
+185
to
+187
| _safe_value(rdMolDescriptors.CalcPMI3, mol_with_3d_conformer), | ||
| _safe_value(rdMolDescriptors.CalcPMI2, mol_with_3d_conformer), | ||
| _safe_value(rdMolDescriptors.CalcPMI1, mol_with_3d_conformer), |
pkacprzak5
force-pushed
the
pk-rdkit-descriptors
branch
from
0ea4a46 to
41f47cd
Compare
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
2 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.
Changes
Short description of changes
Checklist before requesting a review
make test-coverage)make docsand seedocs/_build/index.html)