search

stfc / janus-core

Tools for machine learnt interatomic potentials
https://stfc.github.io/janus-core/
BSD 3-Clause "New" or "Revised" License
13 stars 8 forks source link

geomopt filter_func argument does not support ase.constraints as advertised #311

Open ajjackson opened 4 days ago

ajjackson commented 4 days ago

The docstring for janus geomopt includes

    filter_func : Optional[str]
        Name of filter function from ase.filters or ase.constraints, to apply
        constraints to atoms. If using --vectors only or --fully-opt, defaults to
        `FrechetCellFilter` if available, otherwise `ExpCellFilter`.

but the code does not actually seem to look for functions in ase.constraints. I would like to use the FixSymmetry constraint with a full geometry optimisation. Is there a way to make that work?

ajjackson commented 4 days ago

I wonder if it will be better to have separate arguments selecting the optimising filter and selecting other constraint(s) as these are really independent choices.

ElliottKasoar commented 4 days ago

I think the docstring is wrong, sorry.

Through the Python interface you could pass the actual constraint function, or (untested) the Atoms already constrained.

This is somewhat of a remnant of moving from ASE 3.22.1, where everything was contained within the constraints module, but yes now that they're separate modules, individual arguments sounds like a reasonable addition.

ajjackson commented 4 days ago

Constraints and Filters don't really work the same way, though. I don't see how e.g. unit-cell optimisation with a FixedAtoms constraint could be set up correctly through the current Python API. In #313 you can see how the constraint should be attached.

ElliottKasoar commented 4 days ago

Ok yes, sorry I didn't think through passing the function through carefully enough, I agree that shouldn't work.

Would you not expect the latter suggestion

struct.set_constraint(constraint)
GeomOpt(struct)

to work though?

I'm probably missing something, but I don't see why it should need to be constrained within the optimiser class?

ajjackson commented 4 days ago

Yeah that should work, and would simplify implementation a bit more! Would it have implications for logging etc.?