Skip to content

Extend non_conservative to a 4-way string mode selector in ASE and TorchSim#211

Merged
Luthaf merged 2 commits intomainfrom
nc-stress
Apr 27, 2026
Merged

Extend non_conservative to a 4-way string mode selector in ASE and TorchSim#211
Luthaf merged 2 commits intomainfrom
nc-stress

Conversation

@PicoCentauri
Copy link
Copy Markdown
Contributor

@PicoCentauri PicoCentauri commented Apr 24, 2026

Matches the non_conservative interface already implemented in the LAMMPS metatomic pair style (metatensor/lammps#47).

The non_conservative parameter was a bool in both MetatomicCalculator and MetatomicModel. It is now a string with four accepted values "off" (default), "on", "forces" and "stress".

Contributor (creator of pull-request) checklist

  • Tests updated (for new features and bugfixes)?
  • Documentation updated (for new features)?
  • Issue referenced (for PRs that solve an issue)?

Reviewer checklist

  • CHANGELOG updated with public API or any other important changes?

📚 Download documentation for this pull-request

@PicoCentauri PicoCentauri requested a review from Luthaf April 24, 2026 06:57
@frostedoyster
Copy link
Copy Markdown
Contributor

frostedoyster commented Apr 24, 2026

Is this what we were talking about yesterday? I don't know if we need it because doing either conservatively (forces or stresses) has the same cost as doing both conservatively

I.e. C forces + NC stresses has the same cost as C forces + C stresses

Of course it's still useful if one wants to test the effects of turning one off, but it's such a niche curiosity that IMO one should hack it locally

@Luthaf
Copy link
Copy Markdown
Member

Luthaf commented Apr 24, 2026

My understanding is more that this is about allowing models that don't offer both outputs (for example because they were not trained with stress)

device=None,
variants: Optional[Dict[str, Optional[str]]] = None,
non_conservative=False,
non_conservative="off",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I'd prefer keeping backward compatibility by making this

Suggested change
non_conservative="off",
non_conservative:Union[bool, Literal["forces", "stress"]]=False,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is making the API unclear if one allows "on" or True but to keep backward compatibility I agree we can change this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

And following #210 should we call it "force" or "forces"?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is making the API unclear if one allows "on" or True

Yes, there should only be True, False, "force", "stress", no "on" here!

And following #210 should we call it "force" or "forces"?

I'd say "forces" is fine here, this is what you used in LAMMPS, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay I changed the synopsis.

Yes in LAMMPS we have <on/off/forces/stress>

Copy link
Copy Markdown
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

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

Thanks!

@Luthaf Luthaf merged commit af852c9 into main Apr 27, 2026
10 checks passed
@Luthaf Luthaf deleted the nc-stress branch April 27, 2026 08: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.

3 participants