-
Notifications
You must be signed in to change notification settings - Fork 12
code-cov pmb.setup_lj_interaction #112
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
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.
Thank you for your work here @1234somesh!
There are a few minor points to consider before merging this to main:
- I think that
delete_particle_entryactually is a more general function than its name suggests: it will actually delete any entry with the input name, regardless if that name is a particle, residue, molecule... - There is now somewhat of a code redundancy between
delete_particle_entryanddestroy_pmb_object_in_system. The issue is thatdestroy_pmb_object_in_systemis now both deleting molecules from the espresso system and deleting entries from the pyMBE, which makes pyMBE not very modular.
I suggest the following solution:
- Rename
delete_particle_entrytodelete_entries_in_dfandparticle_nametoentry_name - Adapt the docs of
delete_entries_in_dfto explain that it will delete all entries inpmb.dfwith name =entry_name - Do a CI test that checks that
delete_entries_in_dfworks as intended. - Update the years of license in
testsuite/lj_tests.pyto include 2025 (date of last modification).
I would leave the refactoring of destroy_pmb_object_in_system to a separate PR because it requires of careful refactoring that implies more work than simply making it call your function.
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. Good job with the tests @1234somesh!
Partial fix to #58 and #115.
Added
pmb.dfFixed
setup_lj_interaction