-
Notifications
You must be signed in to change notification settings - Fork 10
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
Enable NEP interface #288
Enable NEP interface #288
Conversation
Can you wait a bit with this until #280 is done? 😃 |
…ue to gpu requirement)
…ove hardcoded labels)
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.
I don't see anything that needs to be improved 😃
Thank you also for making the part with the ref_names and the train/test file more flexible, as well as adding the mock_nep 👍🏻 💟
static_energy_maker=ForceFieldStaticMaker( | ||
force_field_name="NEP", | ||
), |
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.
Once #333 is merged, this has to be set to None
static_energy_maker=ForceFieldStaticMaker( | |
force_field_name="NEP", | |
), | |
static_energy_maker=None, |
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.
Hi @QuantumChemist, maybe it is better to do it in your #333 itself. I think this PR is now ready to be merged at this point.
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.
of course, will adjust it in the other PR then 😃
@QuantumChemist Please approve if you are happy. |
Pull request was converted to draft
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.
I just added one optional suggestion.
Use the glue.xml core potential instead of fitting 2b terms. | ||
glue_file_path: str | ||
Name of the glue.xml file path. | ||
gpu_identifier_indices: list[int] | ||
List of GPU indices to be used for fitting. Only used for NEP fitting. |
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.
Only an optional suggestion to keep the docstrings more consistent:
Use the glue.xml core potential instead of fitting 2b terms. | |
glue_file_path: str | |
Name of the glue.xml file path. | |
gpu_identifier_indices: list[int] | |
List of GPU indices to be used for fitting. Only used for NEP fitting. | |
Use the glue.xml core potential instead of fitting 2b terms. Only used for GAP fitting | |
glue_file_path: str | |
Name of the glue.xml file path. Only used for GAP fitting | |
gpu_identifier_indices: list[int] | |
List of GPU indices to be used for fitting. Only used for NEP fitting. |
and the same for autodelta
(which I cannot include in the suggestion feature)
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.
didnt see the automerge enabled ^^
This PR addresses #54 by adding the necessary functions needed for fitting NEP models and testing on github CI (we will mock NEP fits as fitting explicitly requires GPU)
Changes
calorine
as dependency in pyproject.tomlsrc/autoplex/fitting/common/utils.py
nep
using pytest fixtures (addmock_nep
in conftest.py)