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

Add MeshPotential calculator #3

Merged
merged 19 commits into from
Dec 4, 2023
Merged

Add MeshPotential calculator #3

merged 19 commits into from
Dec 4, 2023

Conversation

kvhuguenin
Copy link
Contributor

@kvhuguenin kvhuguenin commented Nov 30, 2023

Add a first working version containing:

  1. the class MeshPotential that computes the electrostatic potential for one or an entire list of atomic structures at the location of each atom
  2. two auxilary classes (MeshInterpolator, FourierSpaceConvolution) that each perform the two key subtasks

The code comes with

  1. unit tests covering most relevant use cases of the currently implemented functionality
  2. sphinx documentation of all external and internal functions
  3. torchscript compatibility

📚 Documentation preview 📚: https://meshlode--3.org.readthedocs.build/en/3/

Copy link
Contributor

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

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

Some first comments on the docs. I will continue later with more detailed. comments.

.gitignore Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
Copy link
Contributor

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

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

Looks very clean and documentated. I have some comments basically that will help users if the provide wrong shapes of the arrays.

Also, maybe we should also do examples for the FourierSpaceConvolution and the MeshInterpolator classes. What do you think?

tests/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/calculators.py Outdated Show resolved Hide resolved
src/meshlode/fourier_convolution.py Outdated Show resolved Hide resolved
src/meshlode/fourier_convolution.py Show resolved Hide resolved
src/meshlode/mesh_interpolator.py Show resolved Hide resolved
src/meshlode/mesh_interpolator.py Outdated Show resolved Hide resolved
src/meshlode/mesh_interpolator.py Outdated Show resolved Hide resolved
@kvhuguenin
Copy link
Contributor Author

I have now implemented all the changes that you suggested, apart from keeping the test_calculators.py file the way it is. I have tried to go through all the main files again to frequently check that the shapes of the provided tensors are correct and added the `` marks for more readable documentation, with an example file for how to use MeshLODE in examples/ .

@kvhuguenin kvhuguenin self-assigned this Dec 1, 2023
@kvhuguenin kvhuguenin added documentation Improvements or additions to documentation enhancement New feature or request labels Dec 1, 2023
@ceriottm
Copy link
Contributor

ceriottm commented Dec 3, 2023

I added an example of MeshInterpolator and FourierConvolution. As far as I'm concerned, it's good to go (hoping that tests pass...)

@PicoCentauri
Copy link
Contributor

Hmm I am a bit confused by the failure of the jobs. We haven't touched the files and the GH action service seems to run fine.

@PicoCentauri PicoCentauri merged commit c57b6e9 into main Dec 4, 2023
4 checks passed
@PicoCentauri PicoCentauri deleted the potential_calculator branch December 8, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants