-
Notifications
You must be signed in to change notification settings - Fork 19
dpa3 architecture #725
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
base: main
Are you sure you want to change the base?
dpa3 architecture #725
Conversation
|
Thanks a lot for this contribution. We'll go through it and try to help as much as possible to merge it quickly and well - and to keep it working as we are still developing metatomic very actively, so things have a tendency to break ^_^ |
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.
Thank you very much for the contribution @littlepeachs!
A few small comments:
- At the moment, the default dtype for this model is
torch.float64. Is this intended? Otherwise, you can set the default totorch.float32by swapping the order oftorch.float64andtorch.float32in the supported dtypes of the architecture - The architecture is using the
OldCompositionModel. It would be ideal to switch it toCompositionModelright away to avoid more maintenance overhead later. The usage ofCompositionModelcan be taken exactly from thePETarchitecture - We need at least a maintainer for this architecture. You can add maintainers for DPA3 to the
CODEOWNERSfile (top-level of the repository)
The tests look good to me and we would just need to add some documentation. I'd be happy to add that myself (with some help from you @littlepeachs) after we solve these few issues I raised. Otherwise, I think we're pretty close to being able to merge this. Thanks again!
Thank you very much for reviewing our code. We will further refine the modifications and submit. |
|
Hey @littlepeachs, just checking out with you that everything is going well? Did you had time to look into the review comments? |
I am sorry to delay the code update. I have been busy with other work so far. I will update the pull request in quick. |
|
I have updated the code based on the reviewer's suggestion. Could some time be spared to review the code again when available? Thanks a lot! |
|
Thanks @littlepeachs for the work, I've finished a few things and now everything looks fine. Can I ask you to fill the section on "Tuning hyperparameters" in @Luthaf there are issues in @Luthaf and @PicoCentauri let me know what you think about the PR overall |
| @littlepeachs this is where you can tell users how to tune the parameters of the model | ||
| to obtain different speed/accuracy tradeoffs |
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.
Maybe some hints of there or a link to an extrenal docs would be nice.
pyproject.toml
Outdated
| [project.optional-dependencies] | ||
| dpa3 = [ | ||
| "deepmd-kit>=3.1.0", | ||
| "torch>=2.7", |
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.
torch is required by all arches.
| "torch>=2.7", |
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.
deepmd might require torch >= 2.7 specifically though? I could not find anything in the github repository, but I'm not sure about the wheels.
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.
Ahh yes, might be possible.
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 see that deepmd-kit, as released on PyPI, is compiled with C++11 ABI compatibility and therefore fails to load with any torch version before 2.7. In that case, we need "torch>=2.7" here
| architecture: | ||
| name: experimental.dpa3 | ||
| model: | ||
| type_map: [H, C, N, O] |
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.
Isn't this something that we decide internally based on the dataset info @frostedoyster ?
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.
Thanks for spotting this. @littlepeachs this one shouldn't be a hyperparameter, but it should be inferred from the atomic numbers that metatrain will give you (see the other architectures), which you can convert to chemical symbols internally with ase.data.chemical_symbols, if I remember correctly
Contributor (creator of pull-request) checklist
experimentalorstablefolder. See the[docs/src/dev-docs/architecture-life-cycle.rst](Architecture life cycle)
document for requirements.
src/metatrain/experimental/<architecture_name>src/metatrain/experimental/<architecture_name>/default-hypers.yml.github/workflow/architecture-tests.ymloptional-dependenciessection in thepyproject.tomltest_checkpoint.pyin other architectures)Reviewer checklist
New experimental architectures
tests.
TorchScript <https://pytorch.org/docs/stable/jit.html>_.__maintainers__and theCODEOWNERSfilePyPI, a public git repository or another public URL with a repository is acceptable.
New stable architectures
file.
PyPI.
metatrain, includinglogging and model save locations.
📚 Documentation preview 📚: https://metatrain--725.org.readthedocs.build/en/725/