reset_labels kwarg to force_overlap, new method reset_labels

[jaclark5]

PR Summary:

This PR resolves #1194

This PR added the reset_labels kwarg to force_overlap with a default of False. In order to reset the labels as will, that code has been moved to a new method, Compound.reset_labels.

PR Checklist

---

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

[chrisjonesBSU]

Thanks for the PR! Just a note, the macOS-13 tests failing seems to be unrelated to these changes, so we can ignore those for now.

It seems like adding the reset_labels kwarg to force_overlap was enough to fix the issue. So, I'm curious what the train of thought is for moving that section of code to its own method. Are there use cases where we might need to use reset_labels outside of remove? Or is it just for readability and code cleanliness?

[jaclark5]

@chrisjonesBSU, separating rest_labels into it's own method isn't needed for this pull-request, but I think by offering the option to not reset the labels in the moment, should come with it the option to reset them later. An example is when making a monomer, I might have a backbone group with four ports, where I put branches/pendants on two of them and want to control exactly the port I'm using (so renumbering after connecting the first group isn't desired.). I'm then left with two ports which might be port[1] and port[3] (without renumbering), where I might then want the labeling to be cleaned up, so I'll run monomer.reset_labels()

[chrisjonesBSU]

The failing tests should be fixed and the codecov reported once you merge upstream/main.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.35%. Comparing base (e120962) to head (19ce3b4).
Report is 23 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1195   +/-   ##
=======================================
  Coverage   87.34%   87.35%           
=======================================
  Files          62       62           
  Lines        6584     6586    +2     
=======================================
+ Hits         5751     5753    +2     
  Misses        833      833           

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

---

🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[chrisjonesBSU]

LGTM, thanks!

[CalCraven]

Test this with the code:

import mbuild as mb
from collections import OrderedDict
cpd = mb.load("CCCO", smiles=True)
particles_to_remove = list(cpd.particles())[-2:]
cpd.remove(particles_to_remove)
print(cpd.all_ports())
new_labels = OrderedDict()
for i,port in enumerate(cpd.all_ports()):
    new_labels.update({f"port[{i+10}]":port})
    del cpd.labels[f"port[{i}]"]
cpd.labels.update(new_labels)
print(cpd.all_ports())
particles_to_remove = list(cpd.particles())[-2:]
cpd.remove(particles_to_remove, reset_labels=False) # also test with True
print(cpd.all_ports())

And it seems to work okay.

I do get an error if I replace the code

new_labels.update({f"port[{i+10}]":port})

with

new_labels.update({f"port[{i+2}]":port})

969     label = label_pattern.format(count)
971 if not replace and label in self.labels:

--> 972 raise MBuildError(f'Label "{label}" already exists in {self}.')
973 else:
974 self.labels[label] = new_child

MBuildError: Label "port[2]" already exists in .

It looks like there's some assumed behavior with the port numbering that's the cause. Essentially, we remove a particle, so we also remove a bond. When we remove a bond, the [`add` function gets called on the port](https://github.com/mosdef-hub/mbuild/blob/8ce8049900be4c995f753fc31f8f95443ea25ace/mbuild/compound.py#L1517), and the label passed is "port[$]". This can clash with naming conventions, if the labels are not reset at all times on the ports. 

Now, we could add some additional logic to handle this automatically, such as just increment until you find an unused label. Or, just add a little detail to this mBuild error to try setting `reset_labels` to True during a remove or force_overlap call. I'm in favor of that approach, but open to thoughts.

[jaclark5]

@CalCraven @chrisjonesBSU I would normally agree with you, except that the reset_labels feature was added to remove in April with my PR #1173. This current PR is resolving an unintended consequence of that PR to restore the default behavior before April 3rd of this year.

[CalCraven]

@jaclark5 while that's a good point I hadn't considered, I'll offer a counter point.
force_overlap as an mBuild function is covered much more intensively in tutorials, workshops, and recipes we offer in the codebase, as opposed to remove which tends to be a bit more of the backend function for actually manually performing some of these operations one step at a time. While a change to the default behavior for remove might be less disruptive, changing the default behavior for remove_overlap has a potential to break many different pre-built recipes people are using in mBuild.

And to your point, that PR for changing remove is only 4 months old, so many people probably haven't even updated mBuild in that time frame, so even less of a chance much code will get broken.

[jaclark5]

@CalCraven The issue was that it did change force_overlap. During force_overlap the method removed the ports to make a bond, calling ‘remove’ and relabeling the molecules. I’m attempting to restore the expected behavior of all of those resources. On Aug 17, 2024, at 10:59 AM, CalCraven ***@***.***> wrote: @jaclark5 while that's a good point I hadn't considered, I'll offer a counter point. force_overlap as an mBuild function is covered much more intensively in tutorials, workshops, and recipes we offer in the codebase, as opposed to remove which tends to be a bit more of the backend function for actually manually performing some of these operations one step at a time. While a change to the default behavior for remove might be less disruptive, changing the default behavior for remove_overlap has a potential to break many different pre-built recipes people are using in mBuild. And to your point, that PR for changing remove is only 4 months old, so many people probably haven't even updated mBuild in that time frame, so even less of a chance much code will get broken. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[chrisjonesBSU]

Ok, I was confused earlier, but changing labels after calling remove() has only been in the Compound class since April, and only part of one mBuild release that came out in May. The PR that added this set a default value of reset_labels=True which changed the old behavior where the labels weren't changed after calling remove().

In #1173 we agreed that reset_labels=True should be the default for Compound.remove() which then changed the behavior for force_overlap from what it has been for years to what it is now for the last 3-4 months. Setting a default of reset_labels=False in force_overlap is basically restoring what the behavior has always been.

I think we should keep reset_lables=False in force_overlap while still keeping it True in Compound.remove(). But also, with reset_labels being moved to its own method, it might not even be a bad idea to set reset_labels=False in Compound.remove() as well. It's now a workflow where the user can go through their process of changing the topology (e.g. calling force_overlap, removing particles, etc..), then once they are done, call reset_labels manually if they want to.

[jaclark5]

Hey @chrisjonesBSU I think we are on the same page now. If I understand correctly, you think the PR is the way that it should be except that the default, reset_labels=False should be added to Compound.remove()?

[CalCraven]

Hey @chrisjonesBSU I think we are on the same page now. If I understand correctly, you think the PR is the way that it should be except that the default, reset_labels=False should be added to Compound.remove()?

Yep, that's what I'm understanding as well. I think that sounds good to me.

Sorry for the confusion, I think I may have caused that by misspeaking above. Also, just make sure the default in the doc string reflects the argument default as well, since I think in remove_overlaps the doc string says True.

[CalCraven]

LGTM once that remove test is fixed to have the correct reset_labels calls!

[chrisjonesBSU]

Hey @chrisjonesBSU I think we are on the same page now. If I understand correctly, you think the PR is the way that it should be except that the default, reset_labels=False should be added to Compound.remove()?

Sorry I guess I suggested both True and False in my last comment haha. Yeah, let's set the default to False in both Compound.remove() and force_overlap.

I think we'll need a test now that checks for the labels correctly being changed after calling Compound.reset_labels()

[jaclark5]

@CalCraven @chrisjonesBSU it should be all good!