Skip to content

Conversation

@1234somesh
Copy link
Contributor

@1234somesh 1234somesh commented Feb 10, 2025

Partial fix to #58 and #115.

Added

  • Helper method to delete entries from pmb.df

Fixed

  • Warning handling and coverage in setup_lj_interaction

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.

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_entry actually 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_entry and destroy_pmb_object_in_system. The issue is that destroy_pmb_object_in_system is 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_entry to delete_entries_in_df and particle_name to entry_name
  • Adapt the docs of delete_entries_in_df to explain that it will delete all entries in pmb.df with name = entry_name
  • Do a CI test that checks that delete_entries_in_df works as intended.
  • Update the years of license in testsuite/lj_tests.py to 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
pm-blanco previously approved these changes Mar 3, 2025
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. Good job with the tests @1234somesh!

@pm-blanco pm-blanco added the enhancement New feature or request label Mar 3, 2025
@pm-blanco pm-blanco merged commit d3d92ed into pyMBE-dev:main Mar 3, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants