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

Added support for Counterpoise calculation in Gaussian with appropria… #5

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

zarkoivkovicc
Copy link
Contributor

The input files with the counterpoise correction keyword and fragments can now be created.
The syntax is the following:
... --specifications "counterpoise = N fragments = list_of_numbers" ...
The N is the number of fragments in the structure, and the list of numbers assigns a fragment to each atom, in the same order as they are in the XYZ file.
The code will not allow unreasonable combinations of those two parameters (at least some of them).
The 5 appropriate tests are also added.
I could not run python setup.py test because this version of the file is not compatible with python 3.10+ (the collectible module is moved to another module or something like that). Nevertheless, I run every group test manually and they all passed.
I hope this addition is useful.
PS This is my first time contributing to any project, so please check it carefully if you can because I might have messed up something that I am not aware of.

Copy link
Collaborator

@RaphaelRobidas RaphaelRobidas left a comment

Choose a reason for hiding this comment

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

Nice work! See my comments on the code, there are a couple adjustments that would make your addition even better. You didn't "mess up" by any means, don't worry about that. Moving your fragment handling code to the Calculation class will make it useful for more potential features, so I suggest you change that. Other than that, everything is in the right place, just go through the comments and reply if you have questions or you don't agree.

Thanks!

ccinput/packages/gaussian.py Outdated Show resolved Hide resolved
ccinput/calculation.py Show resolved Hide resolved
ccinput/tests/test_gaussian.py Outdated Show resolved Hide resolved
ccinput/tests/test_gaussian.py Show resolved Hide resolved
ccinput/tests/test_gaussian.py Outdated Show resolved Hide resolved
ccinput/tests/test_gaussian.py Outdated Show resolved Hide resolved
ccinput/utilities.py Outdated Show resolved Hide resolved
ccinput/utilities.py Outdated Show resolved Hide resolved
ccinput/tests/test_gaussian.py Outdated Show resolved Hide resolved
@zarkoivkovicc
Copy link
Contributor Author

I think I fixed everything, I agree with most of the comments. In the end I opted to parse fragments separately and to treat counterpoise as any other option.
It was fun to code this, I hope I will come up with more useful additions when I have more time.

@RaphaelRobidas RaphaelRobidas merged commit 0a6e391 into cyllab:main Mar 21, 2023
@RaphaelRobidas
Copy link
Collaborator

Thanks very much for the contribution!

I forgot to write it in my last comment, but you can run all the tests with the pytest command. It avoids the Python error you got.

I will add some addition ideas from my local notes to the roadmap on Github, under projects. That could give you some ideas of additions in the future. Off the top of my head, IRC calculations would be useful.

@zarkoivkovicc zarkoivkovicc deleted the CP_correction_Gaussian branch May 9, 2023 15:22
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