Add other MLPs

[fjclark]

This resolves #102 by adding support for MACE-OFF models (and Egret-1) through OpenMMML.

I've just noticed that MACE is in fact available through conda forge (https://anaconda.org/conda-forge/pymace/files/manage) so I'll update to use this.

As well as general comments, it would be great to have comments on:

• Versioning. I've currently removed the use_ani kwarg and replaced with a more general ligand_intramolecular_mlp kwarg. Consistent with this, I've bumped the version to 3.0.0. Is this acceptable or do we want to try and avoid breaking changes? Any suggestions for how to implement these changes if so? Thanks
• Defaults. Should we now default to e.g. the small MACE model or stick with ANI2x for consistency and speed? Not sure if @djcole56 has any suggestions.
• Default Dask behaviour with processes = False. This results in segfaults as is already warned with ANI2x. Should we directly raise an error if users try to run with processes = False and an MLP, rather than waiting for them to hit a segfault?

Thanks!

[fjclark]

(Noting that the tests involving ANI2x seem a bit flaky)

[djcole56]

Should we now default to e.g. the small MACE model or stick with ANI2x for consistency and speed?

Great - I don't have a strong opinion, but I think stick with ANI2x for the moment for consistency

[bieniekmateusz]

standardise and move this to superclass

[fjclark]

I've added _check_available to the superclass. I've not made it an abstract method, rather left it empty in the superclass so that MLPs like ANI2x which are always available don't have to define the method.

[bieniekmateusz]

similarly, nice functionality, no need for _, we'd want other to use this interface if they want to, also maybe MLFF_CLASS, so you can say MLFF_CLASS['ani2x']

[fjclark]

I've renamed to AVAILABLE_ML_FORCE_FIELD_CLASSES to be as explicit as possible -- please also comment below.

[bieniekmateusz]

Literal is not very extendable, but I guess for now we're not autodetecting available MLP classes

[bieniekmateusz]

maybe Enum so you can later say mlp in AVAILABLE_ML_FORCE_FIELDS

[fjclark]

Thanks. The case for having a literal is that this makes type hinting much easier without forcing the user to supply enums as arguments everywhere, but I think my original naming was misleading. I've renamed the Literal to MLForceFieldName and changed _MLFF_NAME_TO_CLASS to AVAILABLE_ML_FORCE_FIELD_CLASSES so the user can now run mlp in AVAILABLE_ML_FORCE_FIELD_CLASSES. Let me know if that's a reasonable solution

[bieniekmateusz]

Versioning - it always feels quite sudden if you completely remove use_ani and it will confuse users. In theory we should do warnings deprecate. But maybe the solution in the middle is to check if use_ani was passed and throw an error teaching and forcing the users to move to "ligand_intramolecular_mlp". Then we directly tell users to upgrade it and we can remove it in later versions easily.

Dask processes = False - it appears this should be True by default when MLPs particularly. I recall processes start new Dask instances and lead to recursion without __main__, which is why this is off by default. An error sounds like a good workaround.

Nice job, I am happy with everything here, the comments are optional, thanks

[fjclark]

Thanks for the review and sorry for the slow response @bieniekmateusz! I've tried to address all of your comments.

Dask processes = False - it appears this should be True by default when MLPs particularly. I recall processes start new Dask instances and lead to recursion without main, which is why this is off by default. An error sounds like a good workaround.

An error is now raised, and I've corrected the tests as required -- this seems to have fixed the flaky tests.

Versioning - it always feels quite sudden if you completely remove use_ani and it will confuse users. In theory we should do warnings deprecate. But maybe the solution in the middle is to check if use_ani was passed and throw an error teaching and forcing the users to move to "ligand_intramolecular_mlp". Then we directly tell users to upgrade it and we can remove it in later versions easily.

I've updated to raise an error if use_ani is called. As this is still a breaking change, shall we stick with version 3.0.0? Also happy to change to a deprecation warning and keep the major version at 2.

Thanks.