Resolve wrongly merging bond/angle/dihedral types

[jaclark5]

Fixes #4001

Changes made in this Pull Request:

• if statement before calling _removeDupes() to make sure the bond/angle/dihedral type names are tuples, otherwise this step is skipped.

PR Checklist

• Tests?
• [NA] Docs?
• CHANGELOG updated?
• Issue raised/referenced?

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (58bbc44) 93.60% compared to head (a3020ca) 93.60%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4003   +/-   ##
========================================
  Coverage    93.60%   93.60%           
========================================
  Files          193      193           
  Lines        25146    25147    +1     
  Branches      4055     4056    +1     
========================================
+ Hits         23538    23539    +1     
  Misses        1092     1092           
  Partials       516      516           

Impacted Files	Coverage Δ
package/MDAnalysis/core/topologyobjects.py	98.16% <100.00%> (+<0.01%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[hmacdope]

@jaclark5 sorry I have been so slow to review. Would you be able to add a test to check the new behaviour is correct?

[jaclark5]

I could use some help understanding why the test I wrote isn't working. Basically I import a LAMMPS data file with 22 bond types.

1. The first assertion confirms that there are indeed 22 bond types since the bond types aren't defined with tuples an _removeDupes is not called.
2. I then call universe.atoms.bonds.topDict._removeDupes() which will combine bond type 12 and bond type 21 to return 21 bond types. I then assert that the number of bonds does not equal 22.

However, it seems that the TopologyDict attribute dict doesn't actually update to reflect the changes made in step 2.
I added some print statements to the TopologyDict to try to debug this, here is the output from pytest test_topologyobjects.py::TestTopologyGroup::test_bond_no_reversal -vvv

Creating a TopologyDict Instance
Initiating TopologyDict
topDict.keys:         topDict id: 4589672672, topDict.dict id: 4590454144, len(topDict.dict.keys()): 22
Creating a TopologyDict Instance
Initiating TopologyDict
topDict._removeDupes: topDict id: 4589672672, topDict.dict id: 4589549056, len(topDict.dict.keys()): 21
Creating a TopologyDict Instance
Initiating TopologyDict
topDict.keys:         topDict id: 4589672672, topDict.dict id: 4590379904, len(topDict.dict.keys()): 22

Edit: I figured it out, it appears that the class is reinitiated every time the bond types are called. Probably because TopologyGroup.topDict returns TopologyDict(self) instead of checking if it exists. This is a separate issue, should I raise it?

[hmacdope]

I think bondDict and its cacheing is due to be removed with #3777, but I agree this is perhaps a bit of a "dark-arts" part of MDAnalysis. Well done for figuring out your issue.

There is also the related issue of the current system of bonds and even the solution in #3777 only retaining one bond with identical indices (eg only one of 1-2 or 2-1 can exist). This is documented in #3818 and I have been meaning to get on with it as it is definately an issue. I think the semi-conclusion we came to last time was that this is a topology parser issue, IE the parser should sort out the bonds so that there are no duplicates.

Sorry for the info dump but thought I would help out with some context.

[jaclark5]

I'm not sure it will since that PR doesn't touch topologyobjects.py. The issue is that TopologyGroup.topDict is supposed call TopologyDict and then cache it so that it doesn't keep reinitiating a new instance. This could be resolved by setting something like self._topDict = None in TopologyGroup.__init__ and then changing TopologyGroup.topDict from:

@property
@cached('dict')
def topDict(self):
    return TopologyDict(self)

to

@property
def topDict(self):
    if self._topDict is None:
        self._topDict = TopologyDict(self)
    return self._topDict

or just initiate topDict in TopologyGroup.__init__... but I haven't looked through to understand the order of operations that would justify the current pattern with a cached method acting as an attribute instead of defining an attribute to begin with.

[hmacdope]

I think this seems reasonable approach for now, I guess you found that the original setup is error prone and likely still is, but this is a step in the right direction

[jaclark5]

Hey @hmacdope, I was wrong, the issue isn't that the TopologyDict isn't caching correctly in the TopologyGroup, but instead that topologyattrs._Connection.get_atoms reinitializes the TopologyGroup every time you call the AtomGroup.bonds and this is not resolved in #3777 but it could be. Once that is resolved and pushed my tests will pass.

[github-actions]

Linter Bot Results:

Hi @jaclark5! Thanks for making this PR. We linted your code and found the following:

There are currently no issues detected! 🎉

[hmacdope]

@jaclark5 do we still need #3777 or is this now ready for a seperate review? I think the approach taken here can stand on its own. :)

[hmacdope]

Two small comments but other than that I am happy.

[hmacdope]

Thanks @jaclark5 for the fantastic work!

[hmacdope]

@MDAnalysis/coredevs any chance anyone can take a quick peek here also?

[jaclark5]

@jaclark5 do we still need #3777 or is this now ready for a seperate review? I think the approach taken here can stand on its own. :)

@hmacdope Not anymore. I had another test that I removed that also removed that dependency. This other test would call universe.atoms.bonds.topdict._removeDupes() and then again call universe.atoms.bonds.topdict.types() to show that bond types 12 and 21 were combined. However it failed since calling universe.atoms.bonds.topdict._removeDupes() reinitialized the topdict and wouldn't retain the effect of calling universe.atoms.bonds.topdict._removeDupes().

I think this is still something to think about, since it would slow MDAnalysis down to reinitialize the bonds topdict every time it's called. Should I make it another issue or would you want to incorporate that into #3777 ?

[hmacdope]

@jaclark5 do we still need #3777 or is this now ready for a seperate review? I think the approach taken here can stand on its own. :)

@hmacdope Not anymore. I had another test that I removed that also removed that dependency. This other test would call universe.atoms.bonds.topdict._removeDupes() and then again call universe.atoms.bonds.topdict.types() to show that bond types 12 and 21 were combined. However it failed since calling universe.atoms.bonds.topdict._removeDupes() reinitialized the topdict and wouldn't retain the effect of calling universe.atoms.bonds.topdict._removeDupes().

I think this is still something to think about, since it would slow MDAnalysis down to reinitialize the bonds topdict every time it's called. Should I make it another issue or would you want to incorporate that into #3777 ?

@jaclark5 raise an issue, I think we can just move forward with this as it is :)

[jaclark5]

@MDAnalysis/coredevs I would appreciate another review :)

[hmacdope]

@MDAnalysis/coredevs can I get another set of eyes here? If no action in a week I will just go ahead. Sorry we have been a bit slow @jaclark5.

[hmacdope]

Cycling CI for aarch.

[hmacdope]

@jaclark5 just need to move some CHANGELOG entry to 2.6.0

[hmacdope]

@IAlibay can I safely ignore CI failure here?

[IAlibay]

@IAlibay can I safely ignore CI failure here?

Ahh sorry for the delay, I thought I had seen a notification come through 😅

Yeahhh ignore it here, it's not relatetd to this code. It's because of the 1.1 release of contourpy from ~ 3 days ago and the fact that we annoyingly still support 32bit... I'll open a PR.

[hmacdope]

Thanks so much @jaclark5! Great work as always. Great to finally get it in.