-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add an interface to PET #68
Conversation
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.
VERY NICE WORK! I have some comments for the first round of reviews.
EPOCH_NUM_ATOMIC: 1000000000000000000 | ||
SCHEDULER_STEP_SIZE_ATOMIC: 500000000 | ||
EPOCHS_WARMUP_ATOMIC: 250000000 |
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.
Do we really need these hypers if in practice you never change them?
Also, we need a page explaining them as for the other architectures.
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.
We could do this, depending on how keen @serfg is. However, this should not be required for experimental models
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.
No it is not required but maybe makes sense to have this is bit cleaned up.
src/metatensor/models/experimental/pet/utils/systems_to_batch_dict.py
Outdated
Show resolved
Hide resolved
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.
Looks good to me. I am happy to merge this if other reviewers don't mind
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.
MERGE SOON
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.
Some small comments, but looks fine overall!
if len(validation_datasets) != 1: | ||
raise ValueError("PET only supports a single validation dataset") | ||
|
||
if device_str == "gpu": |
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.
this should never arrive to a model. IMO we should convert the device to a torch device before passing it to the train script
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.
You're probably right... or maybe a list of devices. We can open an issue for this, because it concerns all the models
Integration of PET into metatensor-models. This PR is intended to have a proper PET wrapper that accepts the unified data structure of metatensor-models and predicts energies and forces. So that one can run molecular dynamics with pre-trained PET models when this PR is ready.
📚 Documentation preview 📚: https://metatensor-models--68.org.readthedocs.build/en/68/