Enhancement: Heat Flux Function#127
Conversation
orionarcher
left a comment
There was a problem hiding this comment.
LGTM, just request a couple small things. Tagging @abhijeetgangan for a look too.
torch_sim/quantities.py
Outdated
| stress: Per-atom stress tensor components: | ||
| - If is_centroid_stress=False: shape (n_particles, 6) for | ||
| :math:`[\sigma_{xx}, \sigma_{yy}, \sigma_{zz}, | ||
| \sigma_{xy}, \sigma_{xz}, \sigma_{yz}]` | ||
| - If is_centroid_stress=True: shape (n_particles, 9) for | ||
| :math:`[\mathbf{r}_{ix}f_{ix}, \mathbf{r}_{iy}f_{iy}, | ||
| \mathbf{r}_{iz}f_{iz}, \mathbf{r}_{ix}f_{iy}, | ||
| \mathbf{r}_{ix}f_{iz}, \mathbf{r}_{iy}f_{iz}, | ||
| \mathbf{r}_{iy}f_{ix}, \mathbf{r}_{iz}f_{ix}, | ||
| \mathbf{r}_{iz}f_{iy}]` |
There was a problem hiding this comment.
The stress tensor should have shape n_batches, 3, 3, would you convert this to follow that convention?
There was a problem hiding this comment.
Sure, no problem.
tests/test_quantities.py
Outdated
| expected = -torch.tensor([1.0, 4.0, 9.0], device=virial.device) | ||
| assert_allclose(virial.cpu().numpy(), expected.cpu().numpy()) | ||
|
|
||
| def test_batched_calculation(self, device: torch.device) -> None: |
There was a problem hiding this comment.
Could you add a test to check that if I run unbatched(A) and unbatched(B) I get the same results as batched(A, B)?
Does this definition work for potentials that use angles? Since we are using auto-grad to get the stresses it will include all the terms beyond pair-wise interactions. LAMMPS has a warning about that. |
|
@abhijeetgangan thanks for the comments and look-outs!
I thought this might be an issue for general model use, any thoughts on how to handle?
Yeah, the formalism here does not include the domain volume normalization intentionally because I was intending to use it HFACF and for scenarios where the heatflux is calculated for a subset of atoms rather than the whole domain. But that was probably not a suitable reason to exclude it. Can add option to return domain volume normalized J.
Working on a example script that was this.
Hmm, no haven't tested and need to review. The tests I've implemented just check formalism. Thanks! |
Thank you for the PR. This goes well with the correlation PR for Kappa calculations. I was planning to add it myself but wasn't sure because of the limitations I mentioned above. |
|
Did some searching and found a paper by Admal & Tadmor (2015) where they derive heat flux expressions without requiring per-atom energies, using the energy balance law and force–position coupling. But the key issue is that it requires access to partial forces Then I found Carbogno et al. (2017), where they adopt a simplified version using only the virial term: So my thinking is: when per-atom stresses aren’t provided, we extend Let me know what you think? Will proceed if you're on board with that. |
|
@stefanbringuier all good points. I have tested here some of them before. I also merged the LJ to include per-atom quantities in a batch so you can proceed with a example script for both cases where you have per atom quantities vs positions-forces contractions following the script above. I think there is a better way to make this approach more general but for now this should be good enough for a test case. This reference has a detailed derivation of the virial. I would also include the references you mentioned. |
a3b20fb to
e763da9
Compare
|
what's the state of this PR? happy to review if it's ready |
It's still not quite ready as I need to review @abhijeetgangan tests and also work on an example/test using the LJ per-atom quantities vs. the contraction calculation. Will prioritize to bring the PR to a close (been focused on the NEB feature). |
|
ofc, no rush. happy to leave this open. just checking it's not held up by us |
|
Merging for proper credit assignment in advance of #264 |
Summary
calc_heat_fluxfunction to compute heat flux vectorsChecklist
Run ruff on your code.