镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Preserve nonperiodic coordinates in graph conversion - #182

Open
Davis Rens (davrens101) wants to merge 1 commit into
microsoft:mainfrom
davrens101:fix/mixed-pbc-graph-coordinates
Open

Davis Rens (davrens101) wants to merge 1 commit into
microsoft:mainfrom
davrens101:fix/mixed-pbc-graph-coordinates

Conversation

@davrens101

Copy link
Copy Markdown

Summary

Preserve nonperiodic coordinates when GraphConverter prepares mixed-PBC structures. Replace unconditional fractional-coordinate modulo with ASE's atoms.wrap(), matching the existing batch converter's treatment of periodic axes.

For a 10 Å cubic cell with pbc=[True, True, False], Si atoms at z = 1, 1.5 and 11.2 Å, and a 2 Å cutoff, the old converter moves the last atom to z = 1.2 Å. This creates six directed edges and six three-body entries instead of the physically expected two directed edges and no triplets. The same error occurs for a wire with pbc=[True, False, False].

The legacy converter remains the default calculator path (batch_converter=False, direct_graph=False). This therefore changes actual inference inputs, not just a deprecated helper in isolation.

Changes

  • Use atoms.wrap() to respect the per-axis PBC mask.
  • Add one parametrized regression for slab/wire geometries, checking preserved nonperiodic coordinates, periodic wrapping, exact edges and triplet count.
  • Clarify the existing README limitations: geometric correctness does not establish non-bulk model accuracy.

Verification

Base: e59b088ee621cafe0df1b399a27299f3e9092ede.

  • New regression against unchanged source: 2 failed, both because 11.2 Å became 1.2 Å.
  • python -m pytest -q tests/datasets --disable-warnings: 11 passed after the fix.
  • Real native three-body extension and both graph converters exercised on CPU: slab/wire topology now agrees with direct Cartesian separation and the batch converter. Fully periodic control retains six directed edges and six triplets; existing all-nonperiodic fallback tests pass.
  • Actual MatterSimCalculator with a seeded, tiny initialized M3Gnet (no checkpoint download): the remote atom's spurious force disappears; the genuinely nearby geometry retains interactions. This is an execution smoke check, not a pretrained-model accuracy or performance claim.
  • New regression file passes Black 23.3.0, isort 5.12.0 and Flake8 6.0.0. The converter already fails Black/isort and four Flake8 diagnostics at the unchanged base; this patch leaves those unrelated lines alone.

Local runtime: Windows, Python 3.12.10, PyTorch 2.15.0+cpu, torch-geometric 2.8.1, NumPy 2.5.4. Optional workflow dependencies were not all installed; no full-project-suite, CUDA, macOS or hosted-CI pass is claimed.

ASE may choose periodically equivalent representations near cell boundaries; bitwise identity of coordinates or image offsets is not promised. The reproduction uses a nonsingular cell and does not establish support for singular slab/wire cells.

AI assistance was used to investigate and draft this change; the regression and real graph/calculator execution were run locally. No independent human review is claimed.

@davrens101

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree [company=]

@microsoft-github-policy-service

Copy link
Copy Markdown

Davis Rens (@davrens101) the command you issued was incorrect. Please try again.

Examples are:

@microsoft-github-policy-service agree

and

@microsoft-github-policy-service agree company="your company"

This branch has not been deployed

No deployments
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