-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restore MPTrj EDA to working order #130
Conversation
We can find files inside the extxyz zipfile with magmoms stepping through with the debugger but they aren't being read? |
cbar_coords=(0.18, 0.85, 0.42, 0.02), | ||
# annotate each element with its number of magmoms in MPtrj | ||
anno_kwds=tile_count_anno, | ||
# anno_kwds=tile_count_anno, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this have a replacement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tracked in janosh/pymatviz#199
data/mp/eda_mp_trj.py
Outdated
@@ -196,6 +216,7 @@ def tile_count_anno(hist_vals: list[Any]) -> dict[str, Any]: | |||
|
|||
|
|||
# %% plot histogram of number of sites per element | |||
# TODO fix the cbar position and weirdness with 6x10e0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partially addressed in 0d54d68
…t the strange y axis labels on the Cl element tile
…zip (now with same number of frames as original = 1_580_395) assert len(mp_trj_atoms) == 145_923 assert len(df_mp_trj) == 1_580_395
data/mp/eda_mp_trj.py
Outdated
for atoms_list in tqdm(mp_trj_atoms.values(), total=len(mp_trj_atoms)) | ||
for atoms in atoms_list | ||
} | ||
).T.convert_dtypes() # convert object columns to float/int where possible | ||
df_mp_trj.index.name = "frame_id" | ||
assert len(df_mp_trj) == 1_580_312 # number of total frames | ||
assert len(df_mp_trj) == 1_580_395 # number of total frames | ||
if Key.formula not in df_mp_trj: | ||
raise KeyError(f"{Key.formula!s} not in {df_mp_trj.columns=}") | ||
|
||
# this is the unrelaxed (but MP2020 corrected) formation energy per atom of the actual | ||
# relaxation step | ||
df_mp_trj = df_mp_trj.rename(columns={"ef_per_atom": MbdKey.e_form_dft}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When chunked the script runs fairly quickly, two options exist: i) rerun that script to keep the extra labels I didn't save, I think I didn't do this as some of the formation energies looked wrong to me as they were the same as the energy per atom. ii) just grab the reference energies for the compositions and compute it quickly with https://github.com/janosh/matbench-discovery/blob/4d3d33f63a963f0d54f5538af04dd74866456c8f/matbench_discovery/energy.py#L83C5-L83C24
happy to do either, the only lost information is the bandgap in the current file but stuff does need to be recomputed/referenced for e_form etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, leaning towards ii) as it seems slightly less reliant on flawless data provenance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
option ii) implemented in a1f92db and confirmed to yield identical results as before
…rmation energies on the fly using matbench_discovery.energy.get_e_form_per_atom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CompRhys thanks a lot for fixing this script! 👍
Still missing Magmoms and there are some older pmv heatmap arguments that are no longer valid and consequently cause issues