Skip to content

Conversation

@1234somesh
Copy link
Contributor

@1234somesh 1234somesh commented Nov 5, 2024

Added

  • New optional argument backbone_vector enabling to build molecules along an input vector using pmb.create_molecule and pmb.create_pmb_object

@pm-blanco pm-blanco self-requested a review November 5, 2024 10:27
@pm-blanco pm-blanco added the enhancement New feature or request label Nov 5, 2024
@pm-blanco pm-blanco added this to the pyMBE 1.0.0 milestone Nov 5, 2024
@1234somesh 1234somesh closed this Nov 5, 2024
@1234somesh 1234somesh reopened this Nov 5, 2024
Copy link
Collaborator

@pm-blanco pm-blanco left a 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.

Copy link
Collaborator

@pm-blanco pm-blanco left a 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.

Copy link
Collaborator

@pm-blanco pm-blanco left a 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!

@pm-blanco
Copy link
Collaborator

@kosovan @mariusaarsten here we have added the feature to create molecules along an input vector that you both needed for your projects.

@pm-blanco pm-blanco merged commit 8d2a923 into pyMBE-dev:main Nov 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants