Fix Mixed Thermo Type Handling - #1121
Open
kavanase wants to merge 5 commits into
Open
Conversation
`get_entries` replaced `entry.data` wholesale with the requested `property_data` fields, discarding everything the thermo doc served with the entry -- including `oxidation_states`. Nothing downstream noticed while corrections came pre-applied from the server, but any client-side re-application of `MaterialsProject2020Compatibility` then has to re-guess the oxidation states, and gets them wrong for some peroxides and superoxides: mp-761189 picks up a superoxide instead of a peroxide anion correction, shifting its energy by 1.824 eV (0.182 eV/atom).
…ead of it
`additional_criteria or DEFAULT_THERMOTYPE_CRITERIA` dropped the default
thermo type as soon as the caller passed any criteria of their own. A call
such as
get_entries_in_chemsys("Li-Fe-O", additional_criteria={"is_stable": True})
therefore ran with no thermo type filter at all and pooled GGA_GGA+U,
R2SCAN and GGA_GGA+U_R2SCAN entries together, returning the same material
up to three times at mutually inconsistent energies (26 entries where 11
were meant).
Merge the two instead, so an explicit `thermo_types` still wins but every
other criterion narrows the documented default rather than removing it.
Fixes materialsproject#1104. emmet's thermo builder runs with `use_max_chemsys=True`, so a material's ThermoDoc is only emitted by the build for its own chemical system: elemental Cs comes from the `Cs` build, CsI from `Cs-I`, Cs2TiI6 from `Cs-I-Ti`. For the mixed GGA_GGA+U_R2SCAN thermo type the stored `ConstantEnergyAdjustment` is referenced to *that* build's hull, and `MaterialsProjectDFTMixingScheme` chooses its reference functional per chemical system. `get_entries_in_chemsys` pools the docs of every subsystem, so it mixes entries that sit on two different absolute energy scales. For Cs-Ti-I the elemental Cs and Ti docs are anchored on r2SCAN (-25.1 and -12.9 eV/atom) while everything containing I is anchored on GGA(+U), which puts Cs2TiI6 4.574 eV/atom above the hull instead of on it. No single stored adjustment can fix this: a hull-referenced correction is only meaningful relative to the hull it was computed on, and a material belongs to many chemical systems. So take the entries from the phase diagram MP builds for the whole system instead, where the mixing was applied across every subsystem at once. Those entries are self-consistent by construction and reproduce the hull shown on materialsproject.org exactly: hull distances match the served `energy_above_hull` to 0.0 eV/atom with no material missing, across Cs-Ti-I, H-O, Na-Cl, Ba-Ti-O, Sr-Ti-O, Zn-O, Li-Mn-O, Cs-Pb-I, Ag-Bi-I, Y-Ba-Cu-O and Fe. Where MP has no pre-built diagram (a chemical system with no material of that dimensionality, e.g. Ba-Cs-I-Ti), or the entries need reshaping first, fall back to re-applying the mixing scheme here. That is what MP does when it builds a diagram, but it is chemical-system dependent, so it can anchor on a different hull than MP did and it drops entries it cannot place -- for H-O it loses 14 materials and puts H2O2 0.176 eV/atom off. The fallback therefore warns. Queries that cannot be served on one scale at all -- narrowed by `additional_criteria`, or `compatible_only=False` -- now warn instead of silently returning entries that look usable for a phase diagram. Costs about 6 s more for small systems, where the phase diagram download dominates; roughly flat with system size, so large systems are unaffected.
For the mixed GGA_GGA+U_R2SCAN hull, `get_stability` re-ran
`MaterialsProjectDFTMixingScheme` on `PhaseDiagramDoc.all_entries`. Those
hold one already-mixed entry per material, so no material carries both run
types, the scheme finds no GGA(+U)/r2SCAN pairs to match, and it discards
almost everything it is given: for Cs-Ti-I, 48 entries in, 19 out, with all
29 r2SCAN entries and every ternary gone. A user entry for a phase that
really is on the hull then failed with
`ValueError: No valid decomposition found`.
Feed it the un-mixed GGA(+U) and r2SCAN entries instead, which is the input
the mixing scheme is designed for. An r2SCAN entry for Cs2TiI6 now returns
0.0 eV/atom above the hull, and mp-30951 reproduces the served
`energy_above_hull` of 0.111257.
Note this remains a client-side reconstruction, so it can disagree with
`get_entries_in_chemsys` -- which now mirrors MP's own phase diagram -- on
systems where the mixing scheme anchors differently than MP's build did.
Placing a user entry on MP's hull needs both functionals side by side,
which the pre-built diagram does not carry.
This is the same misunderstanding behind the `test_get_stability` skip
("SOMETHING IS OFF HERE FOR THE MIXING SCHEME"). That test still cannot be
re-enabled for the mixed thermo type: its own reference re-mixes
already-mixed entries, and its bare `ComputedEntry` inputs have no
structures for the scheme to match on.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1104
Summary
emmet's thermo builder runs withuse_max_chemsys=True, so a material'sThermoDocis only emitted by the build for its own chemical system; e.g. elemental Cs from a Cs build, CsI from Cs-I, Cs₂TiI₆ from Cs-I-Ti etc. ForGGA_GGA+U_R2SCANthe storedConstantEnergyAdjustmentis hull-referenced to that (chemical sub-system) build, andMaterialsProjectDFTMixingSchemepicks its reference functional per chemical system.get_entries_in_chemsyspools the docs of every subsystem, so it mixes two absolute energy scales; GGA(+U) and r2SCAN. For the Cs-Ti-I example, that is r2SCAN for Cs and Ti chemsyses, and GGA(+U) for I, Cs-I, I-Ti, Cs-I-Ti; resulting in the spurious +4.57 eV/atom formation energy. #1087 flipping the default to the mixed hull is what exposed this issue.Essentially, the DFT-mixing hull-referenced corrections are only meaningful relative to the hulls they were computed on.
The proposed fix here is to take the mixed entries from the phase diagram MP builds for the whole system, where the mixing was applied across every subsystem at once. I also tested the workaround noted in #1104 (re-apply the mixing scheme after mprester pull); it fixes Cs-Ti-I but isn't general, because the mixing scheme is still chemsys-scoped. With this fix, re-mixing remains a fallback where MP has no pre-built diagram (e.g. Ba-Cs-I-Ti) or the entries need reshaping first, with a warning that the result may differ from
materialsproject.org. Queries that can't be served on one scale at all (narrowedadditional_criteria,compatible_only=False) now also warn.Additional minor fixes:
get_entriesdiscardedentry.datawhenproperty_datawas passed, losingoxidation_states.MaterialsProject2020Compatibilityusesoxidation_states(re-guessing if not present) so keeping them is important for consistency. e.g.; it re-guessesmp-761189wrong and gives a 0.182 eV/atom difference in energy relative to MP (superoxide vs peroxide).additional_criteriareplaced the default thermo type rather than narrowing it, so{"is_stable": True}ran unfiltered and pooled all three thermo types.get_stabilityre-mixed already-mixed entries. For Cs-Ti-I it fed 48 entries in and got 19 out, with all 29 r2SCAN entries and every ternary discarded causingNo valid decomposition foundValueErrors. Fixed; now returns 0.0 for Cs₂TiI₆ as expected.Verification:
Hull distances match MP's
energy_above_hullto 0.0 eV/atom with no material missing across Cs-Ti-I, H-O, Na-Cl, Ba-Ti-O, Sr-Ti-O, Zn-O, Li-Mn-O, Cs-Pb-I, Ag-Bi-I, Y-Ba-Cu-O, Fe test systems, plus the no-diagram fallback on Ba-Cs-I-Ti. New testtest_get_entries_in_chemsys_mixed_hullchecks Cs₂TiI₆ is correctly placed on the hull and H-O matching MP values with nothing missing.Behavioural changes to flag:
get_stabilitystill reconstructs mixing corrections locally, so it can disagree withget_entries_in_chemsyson edge cases like Na-Cl-like systems. Placing a user entry on MP's hull needs both functionals side by side, which the pre-built diagram doesn't carry.test_get_stability[GGA_GGA+U_R2SCAN]still can't be un-skipped: its own reference re-mixes already-mixed entries, and its bareComputedEntryinputs have no structures for the scheme to match on.Checklist
ruff.mypy.- [ ] If applicable, new classes/functions/modules haveduecredit@due.dcitedecorators to reference relevant papers by DOI (example)