Skip to content

Add pbc_wrap_batched_and_get_lattice_shifts to handle unwrapped positions in nl#519

Merged
CompRhys merged 9 commits intomainfrom
nbr-fixes
Mar 25, 2026
Merged

Add pbc_wrap_batched_and_get_lattice_shifts to handle unwrapped positions in nl#519
CompRhys merged 9 commits intomainfrom
nbr-fixes

Conversation

@CompRhys
Copy link
Copy Markdown
Member

Addresses #437

@CompRhys CompRhys linked an issue Mar 21, 2026 that may be closed by this pull request
Comment thread torch_sim/transforms.py
wrapped_pos = torch.bmm(cell_per_atom, wrapped_frac.unsqueeze(2)).squeeze(2)
out = torch.where(active_per_atom, wrapped_pos, positions)
shifts = torch.where(active_per_atom, int_shifts, torch.zeros_like(int_shifts))
return out, shifts
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

key idea was just to keep these extra shifts around and adjust the nl shifts accordingly.

@CompRhys
Copy link
Copy Markdown
Member Author

alchemiops can run on CPU but many of our test cases hit it's default max neighbor amounts

"fcc": lambda: bulk("Cu", "fcc", a=3.6),
"hcp": lambda: bulk("Ti", "hcp", a=2.95, c=4.68),
"diamond": lambda: bulk("Si", "diamond", a=5.43),
"bcc": lambda: bulk("Al", "bcc", a=2 / np.sqrt(3), cubic=True),
Copy link
Copy Markdown
Member Author

@CompRhys CompRhys Mar 22, 2026

Choose a reason for hiding this comment

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

too dense lead to nbr explosion exceeding alchemiops padding heuristic

@CompRhys CompRhys requested a review from abhijeetgangan March 22, 2026 20:05
if stress_computed and fe_model_output["stress"].shape != (1, 3, 3):
raise ValueError(f"{fe_model_output['stress'].shape=} != (1, 3, 3)")

# Translating one atom by a full lattice vector should not change outputs.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@alphalm4 This test seemingly passes with the upstream sevennet so I would guess that no additional changes are needed for the precision issue you mentioned at least at the 0.1 meV level (1e-4)

Copy link
Copy Markdown
Contributor

@alphalm4 alphalm4 Mar 25, 2026

Choose a reason for hiding this comment

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

Great! I think the validation condition should be stricter in this line like below,
since I found only using + lattice_vec luckly PASSES even though NL things are not patched.

shifted_state.positions[0] = shifted_state.positions[0] + 10 * lattice_vec

I also checked that the current PR version, together with nvalchemi-toolkit-ops==0.3.0 and vesin[torch]==0.5.3 passes all sevenn+torchsim tests with all four torchsim_nls. Thanks for considering this.

@CompRhys CompRhys merged commit 8d3417a into main Mar 25, 2026
65 of 71 checks passed
@CompRhys CompRhys deleted the nbr-fixes branch March 25, 2026 14:47
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.

Make neighborlist work with both wrapped and unwrapped coordinates

2 participants