-
Notifications
You must be signed in to change notification settings - Fork 12
include backbone vector in create_molecule #99
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
Conversation
pm-blanco
left a comment
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.
Overall, it looks good to me. However, we are missing a unit test to check this new feature. I suggest adding such unit test to testsuite/define_and_create_molecules_unit_tests.py. This unit test should create a molecule using pmb.create_molecule() with an input backbone_vector and check that the backbone of the corresponding molecule created in espresso follows the same vector as the input backbone_vector.
pm-blanco
left a comment
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.
The unit test looks good to me! Once you document the new optional argument backbone_vector in the list of args of the docstring of create_molecule and fix the issue with the additional columns in pmb.df, I think that we can merge this PR.
…_vector to pmb.create_pmb_objetct
pm-blanco
left a comment
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.
Looks good to me. Congrats on your first successful PR @1234somesh!
|
@kosovan @mariusaarsten here we have added the feature to create molecules along an input vector that you both needed for your projects. |
Added
backbone_vectorenabling to build molecules along an input vector usingpmb.create_moleculeandpmb.create_pmb_object