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

Add option to reset child labels in Compound.remove() #1173

Merged
merged 9 commits into from
Apr 4, 2024

Conversation

jaclark5
Copy link
Contributor

@jaclark5 jaclark5 commented Mar 10, 2024

Two changes are made:

  1. Add keyword argument to Compound.remove() for resetting the labels to achieve two goals. First, if all children of some category are removed, the parent category label will inherently be removed, while the ports are also renumbered.
  2. In Compound.add() if a single Compound is added without a label, the Compound.name attribute is used instead of Compound.class.name to align with the default behavior in:
    • lib/atoms/n4.py : line 14
    • conversion.py : line 586, 700
    • lib/recipes/tiled_compound.py : line 139

PR Summary:

Resolves #1172

Using the code in the issue where the last line is now, Molecule.remove(remove_array, reset_labels=True), the results are now:

Before ['C', 'C[0]', 'H', 'H[0]', 'H[1]', 'C[1]', 'H[2]', 'H[3]', 'O', 'O[0]', 'down', 'up']
After Flatten ['monomer', 'H', 'C', 'C[0]', 'H[2]', 'H[3]', 'C[1]', 'H[4]', 'H[5]', 'O', 'O[0]', 'H[6]', 'H[7]']
After Remove H ['C', 'C[0]', 'C[1]', 'O', 'O[0]', 'port', 'port[0]', 'port[1]', 'port[2]', 'port[3]', 'port[4]', 'port[5]']

After

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

Compound.remove() add option to reset labels
Copy link

codecov bot commented Mar 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.32%. Comparing base (df74ac1) to head (d9c2cfa).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1173      +/-   ##
==========================================
+ Coverage   87.28%   87.32%   +0.04%     
==========================================
  Files          62       62              
  Lines        6502     6525      +23     
==========================================
+ Hits         5675     5698      +23     
  Misses        827      827              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@chrisjonesBSU chrisjonesBSU left a comment

Choose a reason for hiding this comment

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

I left one suggestion, but this looks good to me. Thank you @jaclark5!

mbuild/compound.py Outdated Show resolved Hide resolved
Copy link
Member

@daico007 daico007 left a comment

Choose a reason for hiding this comment

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

LGTM! Found a new bug (unrelated to this PR) that we need to fix though when reset_labels=False. The gist is if you removed all particle of a specific type (in this example all H of the ethane6 molecule, there is still the "H" in ethane6.labels. If we try to print it out, it will error out, since all the underlying object has been removed. This is bug in the remove method, basically we need a better of pruning the references to the removed object. This bug was basically fixed with the reset_label=True, since the new label dict was re-generated with only existing object.

mbuild/compound.py Outdated Show resolved Hide resolved
mbuild/tests/test_compound.py Outdated Show resolved Hide resolved
mbuild/compound.py Outdated Show resolved Hide resolved
@CalCraven
Copy link
Contributor

We should potentially add in a future PR a deprecation warning for reset_index

@daico007 daico007 merged commit 2bb047e into mosdef-hub:main Apr 4, 2024
15 checks passed
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.

Loss of meaningful port names and labels
4 participants