Formation energies in ML potentials#3122
Formation energies in ML potentials#3122zulissimeta wants to merge 13 commits intoQuantum-Accelerators:mainfrom
Conversation
|
Can one of the admins verify this patch? |
…simeta/quacc into formation_energy_calculator_option
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3122 +/- ##
==========================================
- Coverage 97.69% 96.88% -0.82%
==========================================
Files 97 97
Lines 4172 4210 +38
==========================================
+ Hits 4076 4079 +3
- Misses 96 131 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…simeta/quacc into formation_energy_calculator_option
Andrew-S-Rosen
left a comment
There was a problem hiding this comment.
Hi @zulissimeta --- this looks like a nice addition to me. I have left a few comments.
I also think I ultimately need to make sure these fairchem tests run on our Princeton HPC clusters on PR because now that there is so much code, relying for it to run on main is going to be a problem long term. It is also difficult to review because the test coverage isn't reflective on reality. On our side, we'll work on doing that before the merge.
There was a problem hiding this comment.
Is this something that is shipped with fairchem that we can access? I assume not but just want to check.
| import yaml | ||
| from huggingface_hub import hf_hub_download | ||
|
|
||
| LOGGER.info("Downloading OMAT24 formation energy references from HuggingFace...") |
There was a problem hiding this comment.
Many HPC clusters do not have access to the external network. Is there a mechanism we can encourage the users to call to run this in advance (e.g. on a login node)?
| # Download the form_elem_refs.yaml file from HuggingFace | ||
| refs_file = hf_hub_download( | ||
| repo_id="facebook/UMA", | ||
| filename="references/form_elem_refs.yaml", |
There was a problem hiding this comment.
This stores it in the current working directory. It seems like this should instead be prefaced by Path(__file__).parent so it's stored in a fixed location.
Also, are we going to be redownloading the YAML every single time a job runs? That seems inefficient if so. It seems like we should be storing this once in and checking if it's there.
| import gzip | ||
| import json | ||
| from pathlib import Path |
There was a problem hiding this comment.
In here and the other function, do we need to keep these imports inside the function or can they be global?
| "mace-mp": -4.097862720291976, | ||
| "sevennet": -4.096191883087158, | ||
| "orb": -4.093477725982666, | ||
| "orb": -3.7420763969421387, |
There was a problem hiding this comment.
Any idea why these test values for orb are being updated throughout the test suite?
Summary of Changes
This PR adds support for formation energy calculations in MLP recipes via the FAIRChem
FormationEnergyCalculatorwrapper. Formation energies are now accessible through two new parameters instatic_job()andrelax_job():use_formation_energy(bool): Enables formation energy calculation (currently FAIRChem UMA withtask_name='omat'only)formation_energy_kwargs(dict): Optional custom kwargs to pass to theFormationEnergyCalculatorKey Changes
Core Implementation (
src/quacc/recipes/mlp/_base.py):use_formation_energyandformation_energy_kwargsparameters topick_calculator()API Exposure (
src/quacc/recipes/mlp/core.py):static_job()andrelax_job()for clarity and IDE autocompleteTesting (
tests/core/recipes/mlp_recipes/test_core_recipes.py):Documentation (
src/quacc/recipes/mlp/__init__.py):Usage Example
Requirements
main).Note: If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.