Skip to content

Accept 1-based atom maps and add featurizer keyword options#5

Open
craabreu wants to merge 4 commits into
NVIDIA-BioNeMo:mainfrom
craabreu:extend-mol-parsing
Open

Accept 1-based atom maps and add featurizer keyword options#5
craabreu wants to merge 4 commits into
NVIDIA-BioNeMo:mainfrom
craabreu:extend-mol-parsing

Conversation

@craabreu

Copy link
Copy Markdown

Accept 1-based atom maps and add featurizer keyword options

Why

Chemprop's make_mol supports keep_h, ignore_stereo, and atom-map-based reordering. This PR is a step towards making cuik-molmaker the default featurizer in Chemprop's CLI.

Summary

  • parse_mol now accepts atom map numbers that form a complete 1-based ordering ([C:1][O:2]), in addition to the existing 0-based support — a strict permutation check replaces the old range check.
  • The existing ordered C++ parameter is now exposed as a Python keyword argument on mol_featurizer / batch_mol_featurizer (previously unreachable from Python since the pybind binding had no named py::args).
  • New keep_h keyword argument on mol_featurizer / batch_mol_featurizer (molecule path): retains explicit hydrogens already written in a SMILES string, without adding any (explicit_H still controls that).
  • New ignore_stereo keyword argument on mol_featurizer / batch_mol_featurizer and batch_reaction_featurizer (CGR path): clears R/S and cis/trans stereochemistry via a single RDKit::MolOps::removeStereochemistry call. This was a deliberate choice over a manual SetChiralTag/SetStereo port (as Chemprop's make_mol does it) — cuik-molmaker's float chirality feature reads RDKit's _CIPCode property, which survives SetChiralTag(CHI_UNSPECIFIED) but not removeStereochemistry; a manual port would silently leave that feature un-cleared.
  • All new parameters default to false/existing behavior and are appended at the end of their function's parameter list, so every existing call site (C++ and Python, positional or keyword) is unaffected.

Backward compatibility

No existing call site changes behavior or stops working. Every new parameter (ordered, keep_h, ignore_stereo) defaults to the prior behavior and is appended at the end of its function's signature, so:

  • Existing positional calls (C++ or Python) still bind to the same parameters as before.
  • Existing keyword calls are unaffected, since no existing argument was renamed or reordered.
  • explicit_H was deliberately left as-is (not renamed to add_h, despite Chemprop naming its equivalent flag that way) to avoid churn on an already-released name.
  • Confirmed via the pre-existing test suites (test_reaction_features.py, test_features.py), which call these functions positionally without the new keywords and still pass.

Test plan

  • tests/python/test_atom_map_order.py — 1-based/0-based atom map ordering, rejection cases, ordered=False
  • tests/python/test_parse_options.pykeep_h atom counts, ignore_stereo on one-hot + float chirality and bond stereo (molecule path), ignore_stereo on CGR atom features (reaction path)
  • Full suite: PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/python -q → 72 passed, 2 pre-existing failures unrelated to this change (test_rdkit2D[fast/best], a scipy.stats version issue)
  • Confirmed no regression in existing positional-argument callers (test_reaction_features.py, test_features.py)

craabreu added 4 commits July 21, 2026 16:56
parse_mol only honored atom map numbers that formed a complete 0-based
permutation. Track the min and max map number instead, and accept the
ordering when the numbers span a contiguous range starting at either 0
or 1, subtracting the minimum when inverting the order.

Anything else (partial mapping, duplicates, non-contiguous numbers, or a
range starting above 1) is still ignored, leaving the atoms in the order
the SMILES parser produced them.

Add tests/python/test_atom_map_order.py covering both numbering
conventions and the rejection cases.
`parse_mol` could already reorder atoms according to their atom map
numbers, but the featurizers always requested it and Python callers had
no way to turn it off.

Thread an `ordered` parameter through `mol_featurizer` and
`batch_mol_featurizer` down to `read_graph`/`parse_mol`, and bind both
functions with named arguments so `ordered` can be passed by keyword.
It defaults to true, so existing callers are unaffected.

Document the option in docs/USAGE.md and cover it in the atom-map
ordering tests.
@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR extends cuik-molmaker's molecule parsing pipeline with three new keyword options — ordered (existing C++ param, now exposed to Python), keep_h, and ignore_stereo — on mol_featurizer/batch_mol_featurizer, plus ignore_stereo on batch_reaction_featurizer, as a step toward making it the default featurizer in Chemprop's CLI. It also broadens atom-map reordering to accept both 0-based and 1-based complete permutations by replacing a simple range check with a proper min/max permutation test.

  • All new parameters default to existing behavior (ordered=True, keep_h=False, ignore_stereo=False), appended at the end of each signature, leaving every existing positional and keyword call site fully unaffected.
  • The 1-based atom-map support uses unsigned min/max tracking plus a range-size check (max - min + 1 == num_atoms) followed by a duplicate-index check, with short-circuit evaluation preventing unsigned underflow on the degenerate path.
  • ignore_stereo deliberately uses RDKit::MolOps::removeStereochemistry to ensure the _CIPCode property read by the float chirality feature is also cleared.

Confidence Score: 5/5

Safe to merge. All new parameters are optional with defaults that preserve prior behavior, and every existing positional and keyword call site is unaffected.

The atom-map permutation check correctly handles 0-based and 1-based maps, partial maps, duplicates, and gaps. Short-circuit evaluation in the range comparison prevents unsigned-integer underflow on the degenerate path. The keep_h and ignore_stereo flags are threaded consistently through both the molecule and reaction pipelines, removeStereochemistry is placed after addHs in the correct order, and the _CIPCode concern is explicitly addressed. Two comprehensive new test files cover all new code paths including edge cases and backward-compatibility positional usage.

No files require special attention.

Important Files Changed

Filename Overview
src/features.cpp Core parsing logic updated: parse_mol now tracks atom-map min/max for 0/1-based permutation validation, adds keep_h via SmilesParserParams::removeHs, and appends ignore_stereo via removeStereochemistry after addHs. Logic is correct and handles edge cases.
src/features.h Header declarations updated with three new optional parameters and matching Doxygen docs. No issues.
src/reaction_features.cpp parse_rxn_side_mol, parse_reaction, and batch_reaction_featurizer each gain ignore_stereo appended at the end; forwarded correctly through the call chain and applied after addHs.
src/cuik_molmaker_cpp.cpp pybind11 bindings for all three functions gain explicit py::arg names and new optional arguments with correct defaults.
tests/python/test_atom_map_order.py New test file covering 0-based and 1-based atom map reordering, rejection of non-conforming maps, ordered=False disabling, and batch_mol_featurizer parity.
tests/python/test_parse_options.py New test file covering keep_h atom counts, keep_h + explicit_H interaction, chirality clearing via ignore_stereo, bond stereo clearing, and the CGR/reaction path.
docs/USAGE.md Documentation for the new ordered, keep_h, and ignore_stereo options is accurate and matches the implementation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Python: mol_featurizer / batch_mol_featurizer
(smiles, ..., ordered, keep_h, ignore_stereo)"] --> B["read_graph
(smiles, explicit_H, ordered, keep_h, ignore_stereo)"]
    B --> C["parse_mol
(smiles, explicit_H, ordered, keep_h, ignore_stereo)"]
    C --> D["SmilesParserParams
removeHs = !keep_h"]
    D --> E["RDKit::SmilesToMol(smiles, params)"]
    E -->|parse failed| Z["return nullptr"]
    E -->|success| F{ordered?}
    F -->|true| G["Scan atoms for molAtomMapNumber
track map_num_min, map_num_max
clear prop on all mapped atoms"]
    G --> H{"Complete permutation?
min <= 1 AND max-min+1 == num_atoms?"}
    H -->|No| I["ordered = false"]
    H -->|Yes| J["Build inverse_order
check for duplicates"]
    J -->|duplicate| I
    J -->|clean| K["mol.renumberAtoms(inverse_order)"]
    K --> L
    I --> L
    F -->|false| L
    L{explicit_H?}
    L -->|true| M["RDKit::MolOps::addHs"]
    L -->|false| N
    M --> N{ignore_stereo?}
    N -->|true| O["RDKit::MolOps::removeStereochemistry"]
    N -->|false| P["return mol"]
    O --> P
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["Python: mol_featurizer / batch_mol_featurizer
(smiles, ..., ordered, keep_h, ignore_stereo)"] --> B["read_graph
(smiles, explicit_H, ordered, keep_h, ignore_stereo)"]
    B --> C["parse_mol
(smiles, explicit_H, ordered, keep_h, ignore_stereo)"]
    C --> D["SmilesParserParams
removeHs = !keep_h"]
    D --> E["RDKit::SmilesToMol(smiles, params)"]
    E -->|parse failed| Z["return nullptr"]
    E -->|success| F{ordered?}
    F -->|true| G["Scan atoms for molAtomMapNumber
track map_num_min, map_num_max
clear prop on all mapped atoms"]
    G --> H{"Complete permutation?
min <= 1 AND max-min+1 == num_atoms?"}
    H -->|No| I["ordered = false"]
    H -->|Yes| J["Build inverse_order
check for duplicates"]
    J -->|duplicate| I
    J -->|clean| K["mol.renumberAtoms(inverse_order)"]
    K --> L
    I --> L
    F -->|false| L
    L{explicit_H?}
    L -->|true| M["RDKit::MolOps::addHs"]
    L -->|false| N
    M --> N{ignore_stereo?}
    N -->|true| O["RDKit::MolOps::removeStereochemistry"]
    N -->|false| P["return mol"]
    O --> P
Loading

Reviews (1): Last reviewed commit: "Add ignore_stereo to batch_reaction_feat..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant