Conversation
ElliottKasoar
added
the
new benchmark
ml_peg/calcs/actinides/plutonium_dioxide/calc_plutonium_dioxide.py
Outdated
Show resolved
Hide resolved
ml_peg/analysis/actinides/plutonium_dioxide/analyse_plutonium_dioxide.py
Outdated
Show resolved
Hide resolved
|
I think we're looking pretty good over here, @ElliottKasoar can you take a look? |
ElliottKasoar
left a comment
ElliottKasoar
left a comment
There was a problem hiding this comment.
Hi @williamdavie, thanks for this! This is generally looking great, I've just left a few comments/suggestions to tidy things up
| ================== | ||
| Actinides | ||
| ================== |
There was a problem hiding this comment.
| ================== | |
| Actinides | |
| ================== | |
| ========= | |
| Actinides | |
| ========= |
Minor nitpick
| atoms.calc = ( | ||
| model.get_calculator() | ||
| ) # must be called each time as number of atoms changes. |
There was a problem hiding this comment.
This shouldn't be necessary for the calculators we currently support? Plenty of our tests have structures with different numbers of atoms.
You may need to do something like calc = copy(calc) within the loop, but otherwise this adds quite a bit of cost, as we need to fully reload the model each for each structure.
|
|
||
| results_to_save = [] | ||
|
|
||
| for atoms in ref_structures: |
There was a problem hiding this comment.
Could you add tqdm to show the progress here?
| atoms.calc = ( | ||
| model.get_calculator() | ||
| ) # must be called each time as number of atoms changes. | ||
| atoms.get_potential_energy() |
There was a problem hiding this comment.
Can you set a charge and spin (multiplicity) before this, as some models e.g. Orb-omol require these to be set. E.g.
# Set default charge and spin
atoms.info.setdefault("charge", 0)
atoms.info.setdefault("spin", 1)
| EV_TO_KJ_PER_MOL = units.mol / units.kJ | ||
|
|
There was a problem hiding this comment.
| EV_TO_KJ_PER_MOL = units.mol / units.kJ |
Unused?
| energy_labels: list[str] = [] | ||
| force_labels: list[str] = [] | ||
| stress_labels: list[str] = [] | ||
| excluded = 0 |
There was a problem hiding this comment.
This doesn't seem to be changed at all, so probably isn't necessary?
| excluded = 0 | ||
| frame_idx = 0 | ||
|
|
||
| for xyz_file in sorted(model_dir.glob("*.xyz")): |
There was a problem hiding this comment.
Aren't the outputs of the calculation written to a single file? In that case you don't need to loop over this, and can use enumerate(frames) or similar instead of manually iterating through frame_index
| e_ref = atoms.info.get("energy_xtb") | ||
| f_ref = atoms.arrays.get("forces_xtb") | ||
| s_ref = atoms.info.get("REF_stress") | ||
|
|
||
| io.write(struct_dir / f"{label}.xyz", atoms) | ||
|
|
||
| if e_ref is not None: | ||
| energies_ref.append(e_ref / natoms) | ||
| energies_pred.append(atoms.get_total_energy() / natoms) | ||
| energy_labels.append(label) | ||
|
|
||
| if f_ref is not None: | ||
| forces_ref.append(f_ref.ravel()) | ||
| forces_pred.append(atoms.get_forces().ravel()) | ||
| force_labels.extend([label] * (natoms * 3)) | ||
|
|
||
| if s_ref is not None: |
There was a problem hiding this comment.
Are there any that don't have reference values and/or predicted values?
I'm not sure if mae works if the lengths of the lists are different/have None, which is possible given the way this is set up
|
|
||
| MODELS = get_model_names(current_models) | ||
| BENCHMARK_NAME = "Plutonium Dioxide" | ||
| DOCS_URL = "https://ddmms.github.io/ml-peg/user_guide/benchmarks/actinides.html#plutonium_dioxide" |
There was a problem hiding this comment.
| DOCS_URL = "https://ddmms.github.io/ml-peg/user_guide/benchmarks/actinides.html#plutonium_dioxide" | |
| DOCS_URL = "https://ddmms.github.io/ml-peg/user_guide/benchmarks/actinides.html#plutonium-dioxide" |
This comes from your Plutonium Dioxide header in the docs rst file, and the space there is converted into a -
| y_label="Predicted Energy / eV / Atom", | ||
| annotation_metadata={"excluded": "Excluded"}, | ||
| ) | ||
| def energy_density(puo2_stats: dict[str, dict[str, Any]]) -> dict[str, dict]: |
There was a problem hiding this comment.
These functions aren't currently being run, so the plots are not generated during analysis. The easiest way is to add them as inputs e.g. test_puo2 (they don't have to be used, since they're fixtures)
There was a problem hiding this comment.
Can you add actinides to docs/source/user_guide/benchmarks/index.rst? Otherwise it won't be built as part of the docs.
3 participants
Pre-review checklist for PR author
PR author must check the checkboxes below when creating the PR.
Summary
Evaluation of energy, forces and stress on Plutonium (IV) Dioxide
Linked issue
Resolves #347
Progress
Testing
Models tested:
New decorators/callbacks