fix: cover CALYPSO element distance radii

[njzjz-bot]

Summary

• retain the existing CALYPSO safe-distance values where defined
• fall back to ASE maintained covalent radii for all other elements
• add an oxygen trajectory regression test

Tests

• PYTHONPATH=tests python -m unittest -v tests.op.test_run_caly_model_devi.TestRunCalyModelDevi.test_parse_traj_supports_elements_outside_legacy_table tests.op.test_run_caly_model_devi.TestRunCalyModelDevi.test_00_parse_traj
• isort --check-only dpgen2/op/run_caly_model_devi.py tests/op/test_run_caly_model_devi.py
• git diff --check

Closes #353

Coding agent: Codex
Codex version: codex-cli 0.149.0
Model: gpt-5.6-sol
Reasoning effort: xhigh

Summary by CodeRabbit

• Bug Fixes

  • Improved trajectory validation for elements not covered by the legacy safety-distance table.
  • Applied the correct nitrogen–nitrogen distance threshold when accepting or rejecting frames.
  • Added a warning when all trajectory frames are rejected during validation.

• Tests

  • Added coverage for additional element support and nitrogen distance filtering.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b1ee5dff-fa96-4f0e-94e2-ac3f08a5bce8

📥 Commits

Reviewing files that changed from the base of the PR and between 6b01f29 and 523cd86.

📒 Files selected for processing (2)

• dpgen2/op/run_caly_model_devi.py
• tests/op/test_run_caly_model_devi.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

parse_traj now supports shared safe-distance data when legacy CALYPSO values are unavailable. It raises FatalError for unknown elements and logs a warning when filtering rejects every frame. Tests cover oxygen fallback lookup and nitrogen distance thresholds.

Changes

CALYPSO safe-distance handling

Layer / File(s)	Summary
Safe-radius resolution and filtering dpgen2/op/run_caly_model_devi.py	parse_traj prefers legacy CALYPSO radii, uses shared safe-distance values as fallback, raises FatalError for unknown elements, and logs when all frames are rejected.
Safe-distance regression tests tests/op/test_run_caly_model_devi.py	Tests verify oxygen fallback support, nitrogen threshold filtering, and warning logging for rejected trajectories.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 523cd

This change extends CALYPSO element-distance handling with a covalent-radius fallback while preserving existing values and adds regression coverage; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the CALYPSO element distance radius fix.
Linked Issues check	✅ Passed	The changes address issue #353 by preserving legacy CALYPSO values, using shared fallback distance data for omitted elements, raising FatalError for unknown elements, and adding regression tests for o…
Out of Scope Changes check	✅ Passed	The code and test changes remain within the linked issue scope. The added warning for fully filtered trajectories is relevant diagnostic behavior.

Full details: Linked Issues check

Explanation

The changes address issue #353 by preserving legacy CALYPSO values, using shared fallback distance data for omitted elements, raising FatalError for unknown elements, and adding regression tests for oxygen and nitrogen.

• Fix all pre-merge checks with AI

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.44%. Comparing base (6b01f29) to head (523cd86).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
dpgen2/op/run_caly_model_devi.py	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #379      +/-   ##
==========================================
+ Coverage   84.43%   84.44%   +0.01%     
==========================================
  Files         104      104              
  Lines        6110     6121      +11     
==========================================
+ Hits         5159     5169      +10     
- Misses        951      952       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Retracted. This review was produced without running the mandated /code-review fan-out (the loop skill's section 2); the substitute process used instead has since been shown to miss findings and, in one case, to state a verified-sounding falsehood. Re-reviewing properly.

[wanghan-iapcm]

The KeyError is real and worth fixing, but the fallback draws from the wrong table, and the result is that the elements this PR is meant to rescue now get silently discarded instead of crashing. One root cause, so one change should fix all of it.

The legacy table is not covalent radii in Bohr

The new comment says it is. It is not — it is a CALYPSO-tuned minimum-safe-distance table. Two independent checks:

1. If it were covalent radii in Bohr, legacy * 0.529 would equal the Cordero radius for every element, i.e. a constant ratio. Instead the ratio spans 0.46 to 1.39 — a factor of three.
2. The legacy table has Na (1.45) < Li (1.50). Every real radius set has Na larger than Li (Cordero/ASE: 1.66 vs 1.28) because Na is a row further down. A radius table in any unit cannot invert that ordering.

So the fallback mixes two different physical quantities under a shared unit label.

What that does, reproduced

N2 at its equilibrium bond length, 1.098 A
  threshold dr(N,N) = 1.1833 A
  parse_traj -> 0 frames

Nitrogen is one of the seven elements #353 names. Before this PR an N-containing trajectory raised KeyError: 'N'; after it, every frame is dropped. And it is silent: parse_traj returns [] rather than None, and the caller only guards is None, so the loop body never runs, no traj.*.dump or model_devi.*.out is written, and nothing is logged. A loud failure has become an invisible one. Same for bcc vanadium, which clears the fallback threshold by 2.8% at ambient and fails once modestly compressed — an ordinary state for a structure search.

The table you want already exists in this repo

dpgen2/exploration/selector/distance_conf_filter.py defines a safe_dist_dict that has the same name, uses the same * 0.529 / 1.2 scaling, documents its unit (# bohr -> ang, and a dargs doc reading "Custom safe distance (in unit of bohr)"), and covers 97 elements H–Cf — including all seven of O, N, F, Cl, Fe, Ni, Cu. Fifteen of the 27 entries in the local table are already numerically identical to it; the two share an ancestor. I checked that importing it creates no circular import.

This is also what #353 asked for: "reuse the covalent-radius data already present elsewhere in the package." Using it needs no rescaling and no new data source.

For the record on how the gap arose: fa4c0db (#225, 2024-06-01) introduced safe_dist_dict, dthresh = 0.72 and this filter together, with no citation and no unit annotation. Ten weeks later 44d3fd1 (#250) wrote the full-coverage version for a different filter in a different module, and nobody backported it.

Two smaller things

CI is red. pre-commit.ci fails on ruff-format; the pinned v0.1.3 wants the dr expression on one line. The PR body lists isort --check-only but not ruff format --check.

Consistency with #383. That PR generates CALYPSO's DistanceOfIon as 0.7 * (r_i + r_j). This one filters at (r_i + r_j) / 1.2 = 0.833 * (r_i + r_j). For O–O that means dpgen2 authorises CALYPSO to generate down to 0.92 A and then discards everything below 1.10 A. Worth agreeing on one source and one factor before either lands.

The rest is inline.