Skip to content

fix(distributed): Fix halo routing for unwrapped periodic migrants - #158

Open
atulcthakur wants to merge 3 commits into
NVIDIA:0.2.0-rcfrom
atulcthakur:fix/periodic-migrant-halo-routing
Open

fix(distributed): Fix halo routing for unwrapped periodic migrants#158
atulcthakur wants to merge 3 commits into
NVIDIA:0.2.0-rcfrom
atulcthakur:fix/periodic-migrant-halo-routing

Conversation

@atulcthakur

Copy link
Copy Markdown
Collaborator

ALCHEMI Toolkit Pull Request

Description

During migration hysteresis, an atom can remain owned by its old rank after crossing a periodic boundary.

For example, in a 20 Å cell, an atom at raw z=25.1 is physically at z=5.1. Rank 0 can temporarily retain ownership, but the halo code checks the raw fractional coordinate 1.255 and fails to send the atom to rank 1. A rank-1 atom at z=5.6 then misses a real 0.5 Å interaction and receives the wrong force.

Fix

Wrap fractional coordinates into the primary cell on periodic axes before deciding which ranks need the atom as a ghost.

The original Cartesian coordinates are still transmitted. The receiver’s neighbor list applies the required periodic image.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Performance improvement
  • Documentation update
  • Refactoring (no functional changes)
  • CI/CD or infrastructure change

Related Issues

Changes Made

  • nvalchemi/distributed/_core/particle_halo.py

    • Canonicalizes periodic fractional coordinates during halo-recipient selection.
  • test/distributed/test_deferred_migration_halo.py

    • Adds four-rank Gloo and NCCL versions of the same force regression.
    • Compares distributed forces against a single-domain calculation at the same physical geometry.

Testing

  • Unit tests pass locally (make pytest)
  • Linting passes (make lint)
  • New tests added for new functionality meets coverage expectations?

Checklist

  • I have read and understand the Contributing Guidelines
  • I have updated the CHANGELOG.md
  • I have performed a self-review of my code
  • I have added docstrings to new functions/classes
  • I have updated the documentation (if applicable)

Additional Notes

Tip

This repository uses Greptile, an AI code review service, to help conduct
pull request reviews. We encourage contributors to read and consider suggestions
made by Greptile, but note that human maintainers will provide the necessary
reviews for merging: Greptile's comments are not a qualitative judgement
of your code, nor is it an indication that the PR will be accepted/rejected.
We encourage the use of emoji reactions to Greptile comments, depending on
their usefulness and accuracy.

Signed-off-by: atulcthakur <atthakur@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@atulcthakur

Copy link
Copy Markdown
Collaborator Author

/ok to test 1fb0a60

@greptile-apps

greptile-apps Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR canonicalizes periodic fractional coordinates during halo-recipient selection while preserving raw Cartesian positions for transmission.

  • Adds direct and periodic-image unit coverage for halo routing.
  • Adds four-rank Gloo and NCCL force regressions for deferred periodic migration.
  • Replaces the NCCL regression’s fixed rendezvous port with pytest’s isolated port fixture.

Important Files Changed

Filename Overview
nvalchemi/distributed/_core/particle_halo.py Canonicalizes periodic fractional coordinates only for recipient selection, leaving transmitted Cartesian coordinates unchanged.
test/distributed/_core/test_particle_halo.py Updates halo-routing expectations and separately covers direct routing of an unwrapped atom and periodic-image routing from the primary cell.
test/distributed/test_deferred_migration_halo.py Adds four-rank force regressions and resolves the previously reported fixed-port collision by using unused_tcp_port.

Reviews (3): Last reviewed commit: "test(distributed): allocate NCCL rendezv..." | Re-trigger Greptile

Comment thread test/distributed/test_deferred_migration_halo.py Outdated
Signed-off-by: atulcthakur <atthakur@nvidia.com>
Signed-off-by: atulcthakur <atthakur@nvidia.com>
@atulcthakur

Copy link
Copy Markdown
Collaborator Author

/ok to test 467c642

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