-
Notifications
You must be signed in to change notification settings - Fork 39
Enhancement: Heat Flux Function #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Enhancement: Heat Flux Function #127
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just request a couple small things. Tagging @abhijeetgangan for a look too.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no problem.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Summary
calc_heat_flux
function to compute heat flux vectorsChecklist
Run ruff on your code.