Basic implementation of geometry optimization interface.

[CedricTravelletti]

This is a basic implementation of the geometry optimization interface. It allows for plug-and-play modifications of the computational backend through the specification of an AtomsCalculators and also for using various optimization libraries via Optimization.jl.

It features a bunch of examples showcasing the above capabilities.

[cortner]

Thanks, Cedric! @tjjarvinen please do a first-round review and I'll then look at it also as soon as I can manage, probably within the next 2-3 days.

[cortner]

We can add a Stillinger-Weber and a Lennard-Jones test either to the examples or to the tests even.

[tjjarvinen]

My main issue is the use of system.particles. AtomsBase has an iterator over particles, so you can mostly just use system instead.

The second point is that AbstractSystem is an abstract type and both FlexibleSystem and FastSystem are it's subtypes. But only FlexibleSystem would work with the code.

[CedricTravelletti]

Thaks @tjjarvinen for the helpful review. Fixes are now implemented.
There is one remaining issue (see my comment about AbstractSystem.

[cortner]

Why is there a type restriction system::AbstractSystem in the first place? Is there any place where we have a dispatch that requires this?

[mfherbst]

Probably not, but I think at least for optimize_geometry is useful to catch sanely programming errors where a user accidentally swaps the argument order, don't you think?

[cortner]

My perspective is that abstract types are not supposed to be used to document, only to organize dispatch. I understand that's not how it is always used (even in Base). This resulted in my having to copy-paste entire functions from Base into my own code and remove the type annotation to be able to use it.

Until there is multiple inheritance I prefer to not restrict type signatures.

[cortner]

Am I right that the big missing gap is the optimization of the cell shape? This is an absolute must I think. Once we have that, and a more general mask, then the question how to organize DOFs becomes a little interesting. In JuLIP I had a DofManager that organized this for me.

[cortner]

I left some comments - we can iterate on what should be done now or can be postponed.

[CedricTravelletti]

Changes implemented. Ready for another round of review.
There are two review suggestions that I am leaving pending:

1. @tjjarvinen The AbstractSystemupdate constructor currently converts everything to FlexibleSystem. I believe that in the future, we should have an update constructor that can update the positions and return the correct system type (fast or flexible).
2. @cortner While I agree that there are different possible tasks when deforming the geometry (saddle point search, ...), usually, when someone talks about geometry optimization, by default they mean minimize the energy. That's also how other DFT packages use the term. I would thus suggest that we keep the optimize_geometry function, and have its default behaviour be to minimize the energy. Then, in the future, we might extend it and dispatch based on the task (saddle point search or other).
   What do you think?

[cortner]

by default they mean minimize the energy

I somewhat agree, but I don't think that this excuses the ambiguity.

Then, in the future, we might extend it and dispatch based on the task (saddle point search or other).

Yes we could define tasks or solvers whatever you want to call them and the run something more generic like solve or optimize

solver = EnergyMinimizer()
solver = DimerMethod() 
optimize(system, solver)

But then the debate just shifts to whether EnergyMinimizer() should be called GeometryOptimizer() (or whatever). So I don't think that will solve the problem.

This isn't a big deal. We have a community package and the community can decide how to name things. If I'm outvoted then that's fine and as author you arguably have a bigger say. But my own proposal remains to be unambiguous and bearing in mind that the first three obvious geometry optimization tasks we will want to support are

• minimize energy
• find a saddle point
• compute the transition path
  I suggest that we think of names that clearly specify these.

[tjjarvinen]

1. @tjjarvinen The AbstractSystemupdate constructor currently converts everything to FlexibleSystem. I believe that in the future, we should have an update constructor that can update the positions and return the correct system type (fast or flexible).

I am fine with this.

From my point all fine with the PR.

[cortner]

Let's get quick feedback from @mfherbst @rkurchin on the naming convention for optimize_geometry before we merge. Maybe @antoine-levitt also has a useful perspective.

If you don't want to reread the thread: I am suggesting that optimize_geometry is ambiguous - the entire package is called GeometryOptimization and will cover e.g. also saddle search and transition paths -- hence this should be called something else, e.g., minimize_energy.

[antoine-levitt]

I guess it depends what is more obvious (and therefore can be left implied), if it's the energy or the geometry. Given the title of the package, @cortner 's suggestion seems reasonable to me.

(I guess when dealing with code or math, I typically think about optimizing the objective function, but when thinking more about the application I would think about optimizing the decision variables.)

(FWIW, when teaching students how to model concrete problems as mathematical optimization they seem to have more trouble figuring out the unknowns than the criterion, but I'm not sure what side, if any, this speaks in favor of)

[CedricTravelletti]

Given the above discussion, I agree with the suggestions and changed optimize_geometry to minimize_energy.

I think all comments are addressed now.

[cortner]

I don't think so. Various comments have been unaddressed and not responded to.

[CedricTravelletti]

@cortner sorry I did not respond to the comments. I now added answers in the thread at the corresponding locations.
To summarize what had been left out:

• clamp_atoms: it was suggested that this should be clamp_atoms!. The problem is that this function relies on AtomsBasewhich currently does not provide in-place modifications. So I think we have to keep it as pure function.
• Type restritction system::AbstractSystem: this has been removed. No type anymore.
• Generalize clamping of atoms: I opened Generalize atoms clamping #2 to track the status of this.
• Unit cell optimization: Working on this now.

I hope this addresses what I had left out.

[rkurchin]

Sorry for slow response here, I'm back online now – mostly LGTM, but I would strongly advocate for separating out constraints, see my comments for more detail.

[mfherbst]

On the minimize_energy versus optimize_geometry debate I think given the name of the package minimize_energy makes it clearer what happens.

[mfherbst]

mostly formatting nits

[cortner]

@CedricTravelletti , thanks for following up. I want to take a quick look at the position and cell optimization before merging if that's ok?

I actually can't find examples or tests for optimizing a supercell with and without the cell shape included in the optimization? Can you point me to where I should look?

[CedricTravelletti]

@CedricTravelletti , thanks for following up. I want to take a quick look at the position and cell optimization before merging if that's ok?

I actually can't find examples or tests for optimizing a supercell with and without the cell shape included in the optimization? Can you point me to where I should look?

Dear Christoph

With Michael we think that the part about cell shape optimization can go in a different PR, since there will probably be much more to discuss on that one.
There is a preliminary version implemented in the lattice branch of the repository, and we are working on it this week to make it correct and benchmarked on realistic problems.

So for now, I suggest that this PR only handles fixed cell optimization and that variable cell optimization is taken care of in a next PR mergin the lattice branch.

[cortner]

Ok.

[cortner]

In that case is it ready?

[rkurchin]

I'm happy for this to be merged! Perhaps we should open an issue regarding the cell shape optimization just to keep visible that that's a to-do item?

[CedricTravelletti]

Yes, the non unit-cell optimization part is ready.