MPMorph Flows

[BryantLi-BLI]

This PR seeks to port the original MPMorph workflows from atomate to atomate2. These workflows are used to generate equilibrated amorphous/crystalline/glassy structures with MD runs via AIMD.

Tagging in @Tinaatucsd, @mcgalcode, @mattmcdermott, @esoteric-ephemera, @vir-k01, @sivonxay because they have used it recently or are apart of the original development team.

Completed

• Volume equilibration workflow meant to equilibrate structure without NPT via a series of NVT runs (Equilibration + production volume)
• Code independent equilibration and production workflow, with optional quench methods
• VASP workflows used in MP papers below recreated in this PR:
  • Hui Zheng et al.
  • Aykol and Persson 2018
  • Murat et al.
• There is an additional feature with this PR to incorporate MLFF MD runs with the same flow.
• Multi quench options available:
• Fast quench (double relax and static)
• Slow quench (series of MD runs to simulate cooling with different temperature profiles)
• Started to add tests for VASP
• MLFF tests are completed
• Pytests
• Packmol conversion to pymatgen interface (used to generated initial amorphous structure)
• Slow quench temperature profiles

MISC

• Seeking feedback for code, missing features, usability

NOTE: Original PR closed and reopened due to some git commit history issues that caused large commit changes. This PR should have resolved the issue. Many thanks to @mcgalcode.

[BryantLi-BLI]

@utf @janosh @JaGeo: Should be ready for review!

[orionarcher]

I found this a bit hard to parse, consider:

            # Fit EOS to the running volume and energy from previous calculations
            self.postprocessor.fit(working_outputs)
            working_outputs = dict(self.postprocessor.results)

            # breaks whole flow here if EOS is not fitted properly
            if working_outputs.get("V0") is None:
                return Response(output=working_outputs, stop_children=True)

            # Check if the number of calculations has reached the maximum
            n_calcs = len(working_outputs["relax"]["volume"])
            max_calcs = self.postprocessor.min_data_points + (self.max_attempts or 1000)
            max_calcs_reached = n_calcs >= max_calcs

            v_0 = working_outputs.get("V0")
            v_min = working_outputs.get("Vmin")
            v_max = working_outputs.get("Vmax")

            # return final structure if calculated volume is in interpolation regime
            if max_calcs_reached or (v_min <= v_0 <= v_max):
                final_structure = structure.copy()
                final_structure.scale_lattice(v_0)
                return Response(output=final_structure)

            # Else, if the extrapolated equilibrium volume is outside the range of
            # fitted volumes, scale appropriately
            v_ref = v_min if v_0 < v_min else v_max
            eps_0 = (v_0 / v_ref) ** (1 / 3) - 1.0

            linear_strain = [np.sign(eps_0) * (abs(eps_0) + self.min_strain)]

[esoteric-ephemera]

tweaked this a little bit - let me know if that looks better?

[orionarcher]

LGTM, I might save intermediate variables for the if statements but ultimately a matter of taste.

[janosh]

typo

[esoteric-ephemera]

Changed the docstr generally since it was confusing

[janosh]

is there any point in having generic Makers as defaults? nothing would happen unless you pass in explicit model/DFT Makers, no? in which case, maybe better not to set defaults at all?

[esoteric-ephemera]

The idea was probably to enforce that these are required, but the workflow itself will fail if relax_maker and static_maker are unspecified. Changed their defaults to None

[janosh]

this make looks like it could benefit from more checking of assumptions and raising helpful error messages if they fail. e.g.

 if isinstance(self.initial_strain, float | int):
    self.initial_strain = (
        -abs(self.initial_strain),
        abs(self.initial_strain),
    )
elif not isinstance(self.initial_strain, tuple) or len(self.initial_strain) != 2:
   raise ValueError(f"initial_strain must be float | tuple[float, float], got {initial_strain}")

[janosh]

typo

[janosh]

maybe add a __post_init__ to raise early if convergence_md_maker, production_md_maker, quench_maker are left as None. based on the doc string sounds like maybe production_md_maker shouldn't be allowed to be None anyway?

[esoteric-ephemera]

Added a __post_init__ to this and the base quench maker classes in the same file

[janosh]

looks like this is missing an @abstractmethod decorator?

[esoteric-ephemera]

Added this decorator to a few other places where I can see it's needed (eos common flows, ASE base classes)

[janosh]

same here, should be marked @abstractmethod

[janosh]

the name convergence_md_maker seems a bit inconsistent. maybe call this equilibrium_volume_maker?

[janosh]

double space after :

[janosh]

md_maker is allowed to be None but looks like make will fail if it is? why not make this a required arg?

[esoteric-ephemera]

See above, this has a __post_init__ for checking this is not None now

[janosh]

i think adding a __post_init__ can be considered boilerplate. if you make md_maker a required arg and the user doesn't pass it, you dataclass should raise a similar error message

def __post_init__(self) -> None:
    if self.md_maker is None:
        raise ValueError("You must specify `md_maker` to use this flow.")

[esoteric-ephemera]

Right - that's there already, unless I'm misunderstanding your suggestion?

[janosh]

i know, i'm suggesting removing it and changing

- md_maker: Maker | None = None
+ md_maker: Maker

to make md_maker a required arg

[esoteric-ephemera]

Ahhh yes sorry! Changed as requested, leaving in the __post_init__'s as well tho

[janosh]

was this change accidental? can't return NotImplementedError here, doesn't match the return type Calculator. should keep the raise and the @abstractmethod decorator

[BryantLi-BLI]

I swapped out return with raise.

I'm a bit confused on the @abstractmethod decorator. It seems that every forcefield that inherits the ASEMaker adapts their calculator as @property. Does this imply that all the calculators should be swapped from @property -> @abstractmethod ? or just the original base class where the calculator should be absent?

[esoteric-ephemera]

whoops typo, think @BryantLi-BLI fixed that

[janosh]

Does this imply that all the calculators should be swapped from @Property -> @AbstractMethod ? or just the original base class where the calculator should be absent?

just the base class should have the @abstractmethod decorator. but all the calculator methods should have the @property decorator

[esoteric-ephemera]

@BryantLi-BLI : that's my bad, the AseMaker.calculator should be both a property and abstractmethod. I accidentally removed this in 00d1434 (was trying to see if there was a nice way to serialize ASE calculators without having to subclass AseMaker, and just forgot to restore a few things)

[janosh]

presumably this line is here to include src/atomate2/common/jobs/mp_avg_vol.json.gz with the PyPI package? probably best to pass the full file path to not auto-include future json.gz file in this dir by accident. but more importantly, that file is 650KB and needs to be downloaded by everyone who installs atomate2 if we merge the current config, whether they use MPMorph flows or not. up to @utf to make a decision here but i would vote against adding that file to the package. better to auto-download it when the user first needs it with a link where to get it if user doesn't have an internet connection.

finally, i looked into that file. looks like it's partially redundant. at least the oxi_state and count keys can likely be removed?

[esoteric-ephemera]

@janosh the oxidation states are I think ignored by default, but the counts are needed. If a given chemical environment doesn't exist in a database, the code looks through subspaces of that chemical environment and does a weighted average of structure volumes. The weighted average depends on the counts

I started rewriting this in parquet to jointly store them - might be better to put the parquet file on OpenData and pull down/cache as needed

[esoteric-ephemera]

OK so for clarity: for the ICSD data, the user can either query from oxidation-state dependent or agnostic data, since ICSD includes manually-assigned charges. I pulled the MP data from summary rather than bonds, so the oxidation state info there is ignored.

That's a slight incongruity that I can change if requested

The counts are needed for the same reason given above (the code will sample subspaces of the total chemical space to perform a weighted averaging of reference volumes/atom if the total chemical space is not present in the dataset)

The data is now pulled / cached from figshare

Stuck with JSON since the parquet speed bump is probably not super relevant here (reading basically once rather than reading many times, and the user has the option to manually specify a DataFrame as input, if generating many random structures)