Skip to content
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

Dispersion #192

Merged
merged 4 commits into from
Aug 31, 2024
Merged

Dispersion #192

merged 4 commits into from
Aug 31, 2024

Conversation

ajhoffman1229
Copy link
Contributor

@ajhoffman1229 ajhoffman1229 commented Aug 23, 2024

Summary

Major changes:

Checklist

This PR would not add any new features to the core code and only contains a notebook showing how to add dispersion to the CHGNet calculator through ASE. I installed pre-commit hooks and used them to format the code in the notebook.

  • All existing tests pass.
  • Tests have been added for any new features/fixes.
  • Docstrings have been added in the Google docstring format.

Copy link
Collaborator

@janosh janosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after pip install torch_dftd dftd4, examples/dispersion.ipynb doesn't run for me. getting a kernel crash from the import cell:

Screenshot 2024-08-24 at 06 57 10

@janosh janosh added docs Improvements or additions to documentation ase Atomic simulation environment labels Aug 24, 2024
@ajhoffman1229
Copy link
Contributor Author

ajhoffman1229 commented Aug 27, 2024

I am not sure that DFT-D4 has a module available on pypi, you may need to use conda or mamba. Can you also install the python extension of DFT-D4? I think you will need conda install dftd4 dftd4-python -c conda-forge per the instructions here. I remember having a similar issue before when I installed the base dftd4 package without the dftd4-python component and tried to import DFT-D4. Without the python part, the import will fail.

Also, did you run pip install torch_dftd or pip install torch-dftd? I believe the latter is what is listed on pypi and I'm not sure the installation will work with an underscore.

@janosh
Copy link
Collaborator

janosh commented Aug 27, 2024

I am not sure that DFT-D4 has a module available on pypi, you may need to use conda or mamba.

i see. not a fan of conda or mamba (strongly prefer uv). but that's not a PR blocker ofc, esp. for an optional dep. @BowenD-UCB you wanna try running the notebook? else happy to merge as is.

PS: pip install torch-dftd and pip install torch_dftd are equivalent.

@ajhoffman1229
Copy link
Contributor Author

I am not sure that DFT-D4 has a module available on pypi, you may need to use conda or mamba.

i see. not a fan of conda or mamba (strongly prefer uv). but that's not a blocker PR ofc, esp. for an optional dep. @BowenD-UCB you wanna try running the notebook? else happy to merge as is.

PS: pip install torch-dftd and pip install torch_dftd are equivalent.

I had not heard of uv before but I can understand the appeal, thank you for sharing! I like the idea of a rust-based environment manager.

@BowenD-UCB
Copy link
Collaborator

Hi @ajhoffman1229 and @janosh
Thanks for the PR!
Sorry for the delay on this.
I tried to install these extra dependencies but encountered installation issue (package incompatibilities) with dftd4-python
As these are optional deps I'll merge as it is now.

@BowenD-UCB BowenD-UCB merged commit 180ffea into CederGroupHub:main Aug 31, 2024
4 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ase Atomic simulation environment docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants