Single responsibility principle

Issue #231 new
andrew_peterson repo owner created an issue

Ask Larsen (from ASE) pointed out that our Amp calculator violates the single responsibility principle. That is, he suggests ASE calculators should only calculate (that is, energy and forces), and anything like training should be in a different class or module. I can see the appeal of this from the coding side, but from the user’s side it’s also convenient to just have one object to deal with. But this is worth considering in future versions of Amp, at least if we’re comfortable with a backwards-incompatible change.

The discussion on ASE’s webpage is here…

https://gitlab.com/ase/ase/-/merge_requests/1834#note_322787774

I guess the user interface would change from:

from amp import Amp

calc = Amp(descriptor=..., model=...)
calc.train(images)

to

from amp.build import CalcBuilder

builder = CalcBuilder(descriptor=, model=...)
calc = builder.train(images)

The second interface seems a bit clumsier to me, and it’s not obvious how you would go about re-training your current calculator object. Nonetheless worth considering, as Ask is usually right. 🙂

Open for comments…

Comments (0)

  1. Log in to comment