Skip to content

Review round 2: fail loudly on bad inputs, report partial results - #51

Merged
niklashoelter merged 1 commit into
masterfrom
fix/review-round2
Oct 28, 2026
Merged

niklashoelter merged 1 commit into
masterfrom
fix/review-round2

Conversation

@niklashoelter

Copy link
Copy Markdown
Owner

Second review round. Each item fails loudly or reports a partial result where gpuma used to carry on silently.

Inputs

  • Multi-XYZ reader: a block whose atom count doesn't match its lines is an error naming the structure and line. Before, it was skipped, or it swallowed the next block. Blank lines between blocks are still fine.
  • Charge and multiplicity are checked against the electron count for SMILES, XYZ and --dry-run. For example, [CH3] as a singlet is now an error.
  • gpuma's own XYZ comments (| Charge: … | Multiplicity: …) are read back and take precedence over the configured values, with a warning when they differ.
  • --xyz-dir: an unreadable file is an error naming the file, and output comments name the source file of each structure.
  • convert and generate check --device. A GPU that doesn't exist no longer falls back to the CPU silently.
  • conformer_seed and the configured multiplicity now reach convert, generate, optimize --smiles and smiles.

Optimization

  • Batch failures: a failed batch retries its structures one at a time, so a bad structure fails only itself. Batch results keep each input's metadata.
  • optimization.max_steps (default 10000) applies to both paths; the ASE path had no limit before.
    • Structures that reach it are returned with converged: false and Not converged in the XYZ comment, and the CLI exits with code 4.
    • In batch mode, convergence is taken from the criterion itself, because torch-sim marks a system that hits the step limit as converged.
  • Non-finite energies or coordinates count as a failed structure, so they never reach the output or the JSON.
  • The ASE path honours energy_convergence_criterion. It uses torch-sim's test, the energy change of one step, instead of switching to the force default.
  • ensemble reports partial (exit 4) when conformers fail, and the summary adds n_generated.

Conformer pruning

  • prune_symmetry falls back to Kabsch RMSD for a molecule RDKit can't kekulize after stripping hydrogens (for example c1cc[cH-]c1). Before, one such molecule aborted the whole library.
  • GetBestRMS gets gpuma's automorphism cap (2e6) as maxMatches. RDKit's default of 1e6 cut off molecules that were still within budget.

Config and smaller fixes

  • New validation:
    • batch_optimization_mode and logging_level must be known values.
    • charge and multiplicity must be integers; 1.5 was silently truncated.
    • steps_between_swaps must be at least 1.
  • model_cache_dir is Fairchem-only: setting it for ORB or SevenNet is an error, and a directory that can't be created is an error. Both are re-checked after --model-type.
  • No output on total failure: when every structure fails, no empty output file overwrites earlier results.
  • GPU selection: a cached model makes its cuda:N current again.
  • ORB precision: ORB's TF32 matmul setting is scoped to ORB loading and ORB runs, instead of applying to the whole process.
  • Summary: model.path names a loaded checkpoint. The log summary and batch comments use it, and the summary counts unconverged structures.
  • .gpuma-version says unknown instead of None when gpuma has no package metadata.
  • --json only gains keys (structures[].converged, n_generated, model.path), so SUMMARY_SCHEMA_VERSION stays 1.

Docs

docs/config.md, docs/cli.md, the agent skill and the example configs (max_steps) are updated.

Testing

On catalyst (4× Quadro RTX 6000, driver 535):

  • Full suite: 772 passed, 1 skipped (that test only runs when nvMolKit is unusable).
  • Each fix has a test that failed before it.
  • The batch failure, retry and step-limit tests use a small harmonic torch-sim model that fails on cue.
  • mkdocs build --strict passes, and the three Python examples run cleanly.
  • The stub-based reporting tests now also pass when run on their own. On master they relied on torch._inductor already being imported by an earlier test.

- Multi-XYZ reader raises on a block that doesn't match its atom count
  instead of skipping it or swallowing the next one.
- Charge and multiplicity are checked against the electron count (SMILES,
  XYZ, dry run); a radical as a singlet is an error.
- gpuma's own XYZ comments (Charge/Multiplicity) are read back.
- prune_symmetry falls back to Kabsch RMSD for a molecule RDKit can't
  kekulize; GetBestRMS gets gpuma's automorphism cap as maxMatches.
- conformer_seed and the configured multiplicity reach convert, generate,
  optimize --smiles and smiles.
- A failed batch retries its structures one at a time; batch results keep
  each input's metadata.
- New optimization.max_steps (both paths); unconverged structures are
  marked (converged, "Not converged") and give exit 4, as do failed
  ensemble conformers. Non-finite energies count as failures.
- The ASE path honours the energy convergence criterion.
- --xyz-dir raises on an unreadable file and names the source files.
- convert and generate check --device.
- Config: batch_optimization_mode, logging_level, integer charge and
  multiplicity, steps_between_swaps >= 1 are validated; model_cache_dir
  is Fairchem-only and must be creatable.
- No empty output file when every structure fails; cached models select
  their GPU again; ORB's TF32 is scoped to ORB runs; the summary names a
  loaded checkpoint (model.path).
@niklashoelter
niklashoelter merged commit 9d5d917 into master Oct 28, 2026
1 check passed
@niklashoelter
niklashoelter deleted the fix/review-round2 branch October 28, 2026 09:09
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