Skip to content
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

bugfix for stress loss #438

Merged
merged 6 commits into from
Jun 4, 2024
Merged

bugfix for stress loss #438

merged 6 commits into from
Jun 4, 2024

Conversation

JPDarby
Copy link
Contributor

@JPDarby JPDarby commented Jun 3, 2024

Removed division of stress by n_atoms in loss to resolve #437

@bernstei
Copy link
Collaborator

bernstei commented Jun 4, 2024

I noticed that the train.py error report says stress_per_atom, which makes me worry - stress is intensive, it shouldn't be "per atom" the way I usually use that phrase. As I poke around the code I see delta_stress and delta_stress_per_atom, e.g.

self.delta_stress.append(batch.stress - output["stress"])

Can someone (@ilyes319 ?) confirm that this kind of bug isn't present elsewhere in the code?

I'm hoping that those variables mean "total stress" and "atomic stress", rather than per_atom meaning divided by n_atoms. However, even if it is, depending on what exactly the atomic stress is (i.e. dE_site / dVol_total, or dE_site / dVol_site (= V_total / n_atoms?), it may or may not be what people are expecting.

@ilyes319 ilyes319 changed the base branch from main to develop June 4, 2024 17:50
@ilyes319 ilyes319 merged commit 7b5ef19 into ACEsuit:develop Jun 4, 2024
2 checks passed
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.

Stress / num_atoms in loss?
3 participants