ASE MD NPT bug fixes + housekeeping

[esoteric-ephemera]

• Closes BUG: issue with running NPT MD #1254 using the changes proposed by @fraricci:
  1. Cell transformation to upper triangular
  2. Berendsen NPT using a different kwarg than Nose-Hoover for the ensemble pressure
     Ensure that the AseMDMaker does not overwrite user-specified kwargs
• Closes MatPesStaticFlowMaker should not have PBE+U step enabled by default #1249 by setting the default MatPES flow to align with the paper
• Closes BUG: OpenFF tests failing #1252 by ensuring openff tests use consistent values of the atomic masses

[JaGeo]

@esoteric-ephemera the openff tests were already failing before, btw

[esoteric-ephemera]

@JaGeo pretty sure that it was #1233, do you know if we can disable dependabot for certain lines of code? That pymatgen dependence is very strict for openff or the tests break completely

[esoteric-ephemera]

@JaGeo part of the openff issue was related to this bug in pymatgen (the mass of Hydrogen was in fact the mass of Tritium)

Solution for more general robustness is to pass the atomic masses as an optional kwarg, as in the tests

[JaGeo]

Thanks @esoteric-ephemera !!

[fraricci]

Hi @esoteric-ephemera.

I did test this pull request and found out that it is not working as expected.

Here are my fixes:
ase/md.py

            # These use different kwargs for pressure
            if (
                isinstance(self.dynamics, DynamicsPresets)
                and DynamicsPresets(self.dynamics) == DynamicsPresets.npt_berendsen
            ) or (
--->           isinstance(self.dynamics, str)
--->           #and issubclass(self.dynamics, MolecularDynamics)
                and self.dynamics == "berendsen"
            ):
                stress_kwarg = "pressure_au"
            else:
                stress_kwarg = "externalstress"

also:

        def _callback(dyn: MolecularDynamics = md_runner) -> None:
            if self.ensemble == MDEnsemble.nve:
                return
            dyn.set_temperature(temperature_K=self.t_schedule[dyn.nsteps])
--->      if self.ensemble in (MDEnsemble.nvt,MDEnsemble.npt):
                return
            dyn.set_stress(self.p_schedule[dyn.nsteps] * 1e3 * units.bar)

Thanks!

[fraricci]

it never enters this if. This fixes it:

isinstance(self.dynamics, str)
#and issubclass(self.dynamics, MolecularDynamics)

[JaGeo]

@fraricci do you want to make a PR and add a small test if possible? Happy to merge. I trust that you checked it carefully.

[esoteric-ephemera]

Ah OK I see - yeah there is extra logic needed for this because the user can either specify self.dynamics as a str or as a MolecularDynamics class. The changes I made should work for the latter case, but not the former like you said

I can roll this into the NEB PR I have open since that's ready to go