Implement geomopt Constraints

[ajjackson]

Closes #311

TODO:

• Testing. Existing tests look fairly simple to extend, is there anything I should know about recommended approach?
• Finalise variable names. constraint_func is used here for consistency with filter_func, but it's a bad name because constraints are actually a class. So are Filter instances; the difference is that Filter is typically called on the Atoms (wrapping them) whereas constraints are attached. From the CLI it would really be nicer if these were "--filter" and "--constraint", and inside the Python API it would be clearer if they were x_class.
• Figure out why filter_kwargs appear in the summary YAML but not constraint_kwargs.

[ajjackson]

@RastislavTuranyi FYI

[ajjackson]

At this point the info is passed into the GeomOpt class. @ElliottKasoar suggests we can simplify things with something like

struct.set_constraint(constraint)
GeomOpt(struct)

in cli.geomopt, instead.

I suppose the main reason not to do this would be that it moves more logic into the (more awkward to test) command-line wrapper, and reduces feature parity between janus geomopt and from janus.calculations import GeomOpt.

If as a project, Janus intends that the Python API is quite similar to the CLI then it might make sense to keep the model here. But if it is expected that the Python API is a bit more streamlined and aimed at expert users, then it would be good to pull the constraint options out of GeomOpt and delegate the information to the incoming structure.

[ElliottKasoar]

That was partly intended as a temporarily solution that I think should work for now, before we merge something like this PR.

For API goals, as long as any features added to the CLI are still possible through the Python, I wouldn't rule things out, and I think this could work either way.

But, in general I'd prefer to keep how we interact with each interface similar (this wasn't quite the driving force, but was a nice benefit of adding reading structure files natively to each calculation class), even at the expense of slightly more code/complexity, so it probably still makes most sense to set the constraint within each Python calculation class, as you have now.

In terms of implementation, I'd imagine eventually we'll want to have a helper function to set the constraint(s?), since we may want to use this elsewhere (e.g. @alinelena used it in MD in the past).

Having the less general implementation just for geom_opt is fine for now though.

[ajjackson]

constraint(s?)

I had wondered about the multiple constraints case, too! Choosing more than one constraint name is not so bad, but the command line syntax could get rather unwieldy if multiple sets of parameter kwargs are also needed.

[ajjackson]

If using the Python API and then selecting a constraint in GeomOpt, should the new constraint remain attached to the input struct after the calculation or would it be better to work on a copy? Or detach it at the end?

I don't really like mutating inputs except when a function only mutates input.

[ElliottKasoar]

constraint(s?)

I had wondered about the multiple constraints case, too! Choosing more than one constraint name is not so bad, but the command line syntax could get rather unwieldy if multiple sets of parameter kwargs are also needed.

The command line could actually be nicer than you might think, since the way lists are passed is by repeating the option e.g. --constaint x --constraint y --constraint-kwargs a --constraint-kwargs b, which feels relatively intuitive to me at least.

If using the Python API and then selecting a constraint in GeomOpt, should the new constraint remain attached to the input struct after the calculation or would it be better to work on a copy? Or detach it at the end?

I don't really like mutating inputs except when a function only mutates input.

I'm definitely open to opinions on what makes most sense and feels consistent with ASE usage generally. Currently the inputs are mutated, but not in a way that should effect future calculations, so I think detaching the constraint would be most consistent with that?

[oerc0122]

constraint(s?)

I had wondered about the multiple constraints case, too! Choosing more than one constraint name is not so bad, but the command line syntax could get rather unwieldy if multiple sets of parameter kwargs are also needed.

The command line could actually be nicer than you might think, since the way lists are passed is by repeating the option e.g. --constaint x --constraint y --constraint-kwargs a --constraint-kwargs b, which feels relatively intuitive to me at least.

If using the Python API and then selecting a constraint in GeomOpt, should the new constraint remain attached to the input struct after the calculation or would it be better to work on a copy? Or detach it at the end?
I don't really like mutating inputs except when a function only mutates input.

I'm definitely open to opinions on what makes most sense and feels consistent with ASE usage generally. Currently the inputs are mutated, but not in a way that should effect future calculations, so I think detaching the constraint would be most consistent with that?

The question is if a constraint takes multiple parameters how are these grouped? Can you build a list of arbitrary tuples?

e.g

--constraint x #noargs# --constraint y --constraint-kwargs a #1arg# --constraint z --constraint-kwargs a b c #3args#

[ajjackson]

I think Janus already requires that other kwargs are provided as string-quoted Python dictionaries. It's a bit ugly but for consistency we should stick with that rather then running into Typer's inability to deal with arbitrary-length options again.

So we end up with
--constraint FixAtoms --constraint-kwargs "{'indices': [1, 2]}" --constraint FixSymmetry --constraint-kwargs "{'symprec': 1e-4}"

which isn't so bad. It's nice to see a good case of the "forcing you to repeat the kwarg" behaviour for a change 😆

[ElliottKasoar]

• Testing. Existing tests look fairly simple to extend, is there anything I should know about recommended approach?

Nothing too specific. I tend to add CLI tests since it effectively tests both the CLI and the Python, but otherwise as long as being able to set the constraint is tested, and ideally error handling is tested etc. too for good coverage, it should be fine.

• Finalise variable names. constraint_func is used here for consistency with filter_func, but it's a bad name because constraints are actually a class. So are Filter instances; the difference is that Filter is typically called on the Atoms (wrapping them) whereas constraints are attached. From the CLI it would really be nicer if these were "--filter" and "--constraint", and inside the Python API it would be clearer if they were x_class.

I'm not against changing both to _class or any other more sensible alternatives in the Python, and happy with just --filter and --constraint for the CLI.

• Figure out why filter_kwargs appear in the summary YAML but not constraint_kwargs.

The summary is built from the optimize_kwargs passed to GeomOpt (GeomOpt(**optimize_kwargs), which includes **minimize_kwargs, in which filter_kwargs is set.

From your docstring in geom_opt.py, it looks like this should work the same way for constraint_kwargs, but at the moment the kwargs are used in the CLI, rather than being passed to GeomOpt.

In general it is also fine to append things to be saved in the summary that aren't directly passed to the calculation class, but in this case I don't think it should be necessary.

[alinelena]

we will need an example, check stfc/janus-tutorials

[ElliottKasoar]

Hi @ajjackson, just to check, is there anything you need help with/clarifying? It would be great to have this merged!

I'm not against changing both to _class or any other more sensible alternatives in the Python, and happy with just --filter and --constraint for the CLI.

To add context, I think originally I was planning to just use filter in the Python, but changed it to avoid clashing with the built-in, so it definitely seems reasonable in the CLI where that's not an issue.

[ajjackson]

I think the only blocker here is my availability 😭