Conversation
ElliottKasoar
added
the
new benchmark
ElliottKasoar
left a comment
ElliottKasoar
left a comment
There was a problem hiding this comment.
Hi @ttompa, thank you so much for this, there's a huge amount of work here, and it looks great so far having done a couple of passes of the code.
I'm in the process of running a few models to test things out in full, so I may return with a few more comments, but so far most of my suggestions/questions are pretty minor.
One other small question I have is whether you think physicality is the most appropriate category. Generally we're quite loose with exactly what fits where, but more often than not this category checks less numerical quantities. Do you think it might fit better somewhere like with bulk crystals?
| f"{ts_result['max_traction']:.2f} GPa" | ||
| ) | ||
|
|
||
| # Save all results as JSON |
There was a problem hiding this comment.
It might be useful here to save the structures involved in these calculations, as we may want to visualise these as part of the app?
| # Unit Conversion Constants | ||
| # ============================================================================= | ||
|
|
||
| EV_TO_J = 1.60218e-19 |
There was a problem hiding this comment.
Generally I'd encourage using ase.units for non-trivial values e.g. in this case 1 / units.J, I believe
There are also values for Angstrom etc. too
| return E0, B0, Bp, V0 | ||
|
|
||
|
|
||
| def fit_eos( |
There was a problem hiding this comment.
Just to check, are you aware of ASE's implementation of a lot of this? https://ase-lib.org/ase/eos.html (and if you wanted to use that, janus-core provides some further wrapping which may be useful).
Assuming you need to do things this does not do, it may be worth adding to the various docstrings to make it clear what is unique about your implementation
There was a problem hiding this comment.
Thanks, I wasn't aware, the ASE version is identical so I just swapped to that and removed the duplicates
|
|
||
| def relax_volume_isotropic( | ||
| atoms: Atoms, | ||
| calc: Any, |
There was a problem hiding this comment.
We use Any in ml_peg in a couple of places since the "Calculator" doesn't start as an ASE calculator, but I think at this point it probably should be an actual ASE calculator, so we can be more specific here?
| opt = BFGS(atoms_relaxed, logfile=None) | ||
| try: | ||
| opt.run(fmax=BFGS_FMAX, steps=BFGS_MAX_ITER) | ||
| except Exception: |
There was a problem hiding this comment.
It would be good to output this exception as a warning
| opt = BFGS(atoms, logfile=None) | ||
| try: | ||
| opt.run(fmax=BFGS_FMAX, steps=BFGS_MAX_ITER) | ||
| except Exception: |
There was a problem hiding this comment.
It would be good to output this exception as a warning
| for column in metrics_df.columns: | ||
| if column == "Model": | ||
| continue | ||
| values = [ | ||
| value if pd.notna(value) else None for value in metrics_df[column].tolist() | ||
| ] | ||
| metrics_dict[column] = dict(zip(metrics_df["Model"], values, strict=False)) |
There was a problem hiding this comment.
If these were not the same length (which would be the reason not to use strict=True), would they always be correctly aligned?
There was a problem hiding this comment.
They are the same length, updated it to strict=True
| return pd.read_csv(csv_path) | ||
|
|
||
|
|
||
| def _create_figure(df: pd.DataFrame, curve_type: str, model_name: str) -> go.Figure: |
There was a problem hiding this comment.
In general we don't really want to do this as part of the app. The idea is these are all pre-created during Analysis, so then we can just load them at this stage, minimising the work done as part of the booting the app.
| if "a0" in eos: | ||
| a0_mlip = eos["a0"] | ||
| a0_error = abs(a0_mlip - DFT_REFERENCE["a0"]) / DFT_REFERENCE["a0"] * 100 | ||
| metrics["a0 error (%)"] = a0_error |
There was a problem hiding this comment.
You don't need the units in brackets for these since it's automatically added from the metrics.yml file
| energies = [] | ||
|
|
||
| for lat in lattice_params: | ||
| atoms = bulk("Fe", "bcc", a=lat, cubic=True) |
There was a problem hiding this comment.
There may be other places where you need to do this, but can you set a default spin (multiplicity) and charge to the atoms.info? These need to be set for Orb-omol, unfortunately
save configs for visualisation, use ASE units, use ASE eos implementation, Any->Calculator type hints, output exceptions as warnings, create figures during analysis, remove duplicate units in metrics, set default spin and charge for Orb-omol compatibility
|
Thanks for reviewing this @ElliottKasoar! I've addressed the comments and re-tested it after the changes to make sure I didn't break anything accidentally |
2 participants
Pre-review checklist for PR author
PR author must check the checkboxes below when creating the PR.
Summary
This adds a set of benchmarks related to iron plasticity and fracture, based on this paper
Linked issue
Resolves #344
Progress
Testing
mace-omat-0
orb-v3-consv-inf-omat
New decorators/callbacks