Skip to content

fix(mpnn): honor parser overrides for AtomArray inputs - #389

Merged
AndrewKubaney merged 5 commits into
RosettaCommons:productionfrom
daylight-00:fix/mpnn-atom-array-parser-args
Oct 16, 2026
Merged

AndrewKubaney merged 5 commits into
RosettaCommons:productionfrom
daylight-00:fix/mpnn-atom-array-parser-args

Conversation

@daylight-00

@daylight-00 daylight-00 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

An AtomArray passed to MPNNInferenceInput.from_atom_array_and_dict() is parsed with the parser defaults: remove_ccds and remove_waters in the input dict are ignored, and there is no way to switch off add_missing_atoms. A ligand whose label is the CCD code of a different molecule then cannot be kept as given: its atom names do not match that template, the parse raises, and the input dict offers no way around it:

# phenytoin (C15 N2 O2, 19 heavy atoms) labelled "PHT", which is phthalate in the CCD
from_atom_array_and_dict(atom_array=array, input_dict={})
# ValueError: Atom 'O1' in residue PHT (chain B, res_id 1) matches neither standard nor alternative CCD names
from_atom_array_and_dict(atom_array=array, input_dict={"add_missing_atoms": False})
# parsed ligand: the 19 atoms as given

The structure-path route already applies these overrides; only the AtomArray route dropped them, although examples/all.ipynb already passes remove_waters next to an AtomArray (with the parser's default value, so nothing showed).

  • The parser fields a caller sets are forwarded to parse_atom_array() through the helper build_atom_array() now shares, so the two routes cannot drift apart again. Fields left unset keep the parser defaults.
  • New optional add_missing_atoms (JSON field and --add_missing_atoms), unset by default, which still means the parser default on both routes.
  • An AtomArray input behaves differently only when its dict sets remove_ccds or remove_waters, which were ignored. The defaults are untouched: they differ between the routes (remove_ccds=[] for a path, crystallization aids removed for an AtomArray), so the defaulted dict values are deliberately not forwarded.
  • An AtomArray whose parse returns no assembly still falls back to structure_path.

Tested with nine new cases and the extended CLI case in models/mpnn/tests/test_inference_utils.py (CPU, AtomWorks 3.0.1). Six fail on production: the PHT case, the two ignored fields, both forwarding checks, and the new field's validation and CLI flag. The checks that pin the unchanged behavior pass on both. All of models/mpnn/tests passes (130), and the repo's pinned ruff and mypy report nothing new.

`MPNNInferenceInput.from_atom_array_and_dict()` parsed an AtomArray with the parser defaults and ignored `remove_ccds` / `remove_waters`, so a ligand whose label is the CCD code of another molecule (phenytoin labelled `PHT`) was rebuilt from that template.

- Forward the parser fields the caller set to `parse_atom_array()` through a helper that `build_atom_array()` now shares; unset fields keep the parser defaults.
- Add `add_missing_atoms` (JSON field and `--add_missing_atoms`, unset by default) so the atoms of an input can be kept as given.
- Test that `add_missing_atoms=False` keeps all 19 phenytoin atoms, that the AtomArray defaults are unchanged, that both routes forward the overrides, and that an input without an assembly still needs `structure_path`.
Copilot AI balanced review requested due to automatic review settings October 2, 2026 07:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

AtomWorks 3 rejects a ligand whose atom names do not match its CCD template, so the defaults check can no longer carry the mislabelled phenytoin.

- Check the defaults on glycerol and water only; the phenytoin case stays in the `add_missing_atoms=False` test.
- Build the expected array with the CCD charges that `from_atom_array_and_dict()` now adds before parsing.

@AndrewKubaney AndrewKubaney left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks for making this fix!

@AndrewKubaney
AndrewKubaney merged commit d078b24 into RosettaCommons:production Oct 16, 2026
5 checks passed
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.

3 participants