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 table potential class to forcefields library #65

Merged
merged 21 commits into from
Oct 3, 2023
Merged

Add table potential class to forcefields library #65

merged 21 commits into from
Oct 3, 2023

Conversation

chrisjonesBSU
Copy link
Member

@chrisjonesBSU chrisjonesBSU commented Sep 27, 2023

This PR adds a class that creates hoomd table potentials. In hoomd, the table potentials are just created from arrays (i.e. you don't give hoomd the txt or npy file). So, this class is designed to accept dictionaries of {type: {}} where the inside dictionary has keys and values of {parameter: parameter_value} for all of the hoomd parameters (force, energy, etc...). However, since the use case is most commonly going to be loading table potential files, I added a from_files class method that handles building up of the these dictionaries.

To Do:

  • Doc strings
  • Unit tests
  • More type checks and errors/warnings

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #65 (05c13b6) into main (133d7d8) will decrease coverage by 0.22%.
Report is 18 commits behind head on main.
The diff coverage is 91.05%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
- Coverage   93.75%   93.54%   -0.22%     
==========================================
  Files          20       20              
  Lines        1410     1533     +123     
==========================================
+ Hits         1322     1434     +112     
- Misses         88       99      +11     
Files Coverage Δ
hoomd_organics/library/__init__.py 100.00% <ø> (ø)
hoomd_organics/library/forcefields.py 94.30% <91.05%> (-5.70%) ⬇️

hoomd_organics/library/forcefields.py Outdated Show resolved Hide resolved
pair_dict[pair_type] = dict()
pair_dict[pair_type]["U"] = table[:, 1]
pair_dict[pair_type]["F"] = table[:, 2]
if len(r_min) != len(r_max) != 1:
Copy link
Member Author

@chrisjonesBSU chrisjonesBSU Sep 27, 2023

Choose a reason for hiding this comment

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

Technically, we can handle pair tables with different r_mins and r_maxs if we set the rmax on a per-pair basis in _create_forcefield

hoomd_organics/library/forcefields.py Show resolved Hide resolved
@chrisjonesBSU chrisjonesBSU marked this pull request as ready for review October 1, 2023 23:23
Copy link
Collaborator

@marjanalbooyeh marjanalbooyeh left a comment

Choose a reason for hiding this comment

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

Everything looks good. I just added a few comments mostly around checks and validations when loading the files, which I think you already have those in mind.
Thank you

bonds: dict, optional, default None
angles: dict, optional, default None
dihedrals: dict, optional, default None

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably it's worth mentioning r_min in the docstrings. I think it's the first time we have it in hoomd-organics

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I forgot that I need to finish this part of the doc strings still.

hoomd_organics/library/forcefields.py Show resolved Hide resolved
hoomd_organics/library/forcefields.py Show resolved Hide resolved
@chrisjonesBSU chrisjonesBSU merged commit 317a353 into cmelab:main Oct 3, 2023
1 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants