BUG: combine cmaps when merging Universes (Issue #3672)#5413
Open
apoorva-01 wants to merge 1 commit into
Open
Conversation
Merge() handles the connection attributes bonds, angles, dihedrals and
impropers through a dedicated path that remaps their atom indices onto
the merged Universe; every other topology attribute is concatenated as a
plain array. cmaps is a connection attribute as well, but it was missing
from that set, so a Universe carrying cmaps fell through to the array
path and raised
TypeError: Encountered unexpected topology attribute of type
<class 'MDAnalysis.core.topologyobjects.TopologyGroup'>
Add cmaps to the set so it is merged the same way as the other
connection groups.
Issue MDAnalysis#3672
IAlibay
requested changes
Jun 29, 2026
IAlibay
left a comment
IAlibay
left a comment
Member
There was a problem hiding this comment.
Thanks for opening this PR. Please fill in the AI disclosure that can be found in the original PR template before we review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5413 +/- ##
========================================
Coverage 93.85% 93.85%
========================================
Files 182 182
Lines 22509 22509
Branches 3202 3202
========================================
+ Hits 21125 21126 +1
+ Misses 922 921 -1
Partials 462 462 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3672
Merge()remaps the connection attributesbonds,angles,dihedralsandimpropers, but treats everything else as a plain array.cmapsis a connection attribute too, so a Universe with cmaps hit the array path and raisedTypeError: Encountered unexpected topology attribute of type ...TopologyGroup."cmaps"to the connection-attribute set inMerge().test_merge_with_cmaps) that fails without the fix.Note:
UreyBradleysis also a_Connectionmissing from that set (same latent bug). I kept this PR to the narrowedcmapsscope; happy to generalise (derive the set from_Connectionsubclasses) or do a follow-up if preferred.LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS?Developers Certificate of Origin
I certify that I can submit this code contribution as described in the
Developer Certificate of Origin, under
the MDAnalysis LICENSE.