mdanalysis icon indicating copy to clipboard operation
mdanalysis copied to clipboard

cmaps are not correctly merged with Merge()

Open ksy141 opened this issue 2 years ago • 3 comments

Is your feature request related to a problem?

I don't think cmaps is properly considered as TopologyObjects.

u1 = mda.Universe('xxx.pdb')
u2 = mda.Universe('xxx.pdb')

u1.add_TopologyAttr('cmaps', [[0,1,2,3,4]])
u2.add_TopologyAttr('cmaps', [[4,5,6,7,8]])
mda.Merge(u1.atoms, u2.atoms)

run into the error: TypeError: Encountered unexpected topology attribute of type <class 'MDAnalysis.core.topologyobjects.TopologyGroup'> (no numpy)

Describe the solution you'd like

Simply adding 'cmaps' in the line 1522 of MDAnalysis/core/universe.py will fix this. before: tops = set(['bonds', 'angles', 'dihedrals', 'impropers']) after: tops = set(['bonds', 'angles', 'dihedrals', 'impropers', 'cmaps'])

Describe alternatives you've considered

I think the above is simple and a proper way to handle this.

Additional context

ksy141 avatar May 20 '22 15:05 ksy141

Apparently, cmaps already exists as a format specific attribute. It should be possible to add it as a topology attribute.

(Given that we have it, then other parsers should read it (namely PSF and TPR, maybe PARMTOP, too).)

Note that the line https://github.com/MDAnalysis/mdanalysis/blob/5dc774784ba31ae04f18f9baf59ccfddd617fa74/package/MDAnalysis/core/universe.py#L1522 is in Merge() and only applies when building universes with the functionality. Is your problem related to the Merge() function and not about having cmaps in a Universe?

orbeckst avatar May 21 '22 08:05 orbeckst

Hi Oliver,

Thank you for your email, and thank you for clarifying my question.

First, currently, you cannot Merge two Universes with cmaps (returning error messages). If either has cmaps, a merged universe will have no cmaps. Therefore, adding cmaps in the Merge function will be a simple fix as I proposed in the original message.

Second, I wonder whether cmaps can be considered TopologyObjects instead of TopologyAttributes. It makes more sense to me that they are considered TopologyObjects as they are not very related to atomic / atomic group / residue / segment properties. I hope they are equally treated as Bonds, Impropers, Dihedrals, and Impropers and have an add_cmaps function.

I think the first one somehow should be fixed, but the second one depends on what you and the community think. Thank you for developing MDAnalysis. It has been the most powerful and useful tool for me in simulation preparation and analysis.

Best, Siyoung

On May 21, 2022, at 3:27 AM, Oliver Beckstein @.***> wrote:

Apparently, cmaps already exists as a format specific attribute https://userguide.mdanalysis.org/stable/topology_system.html#format-specific-attributes. It should be possible to add it as a topology attribute https://userguide.mdanalysis.org/stable/topology_system.html#add-topologyattrs.

(Given that we have it, then other parsers should read it (namely PSF and TPR, maybe PARMTOP, too).)

Note that the line https://github.com/MDAnalysis/mdanalysis/blob/5dc774784ba31ae04f18f9baf59ccfddd617fa74/package/MDAnalysis/core/universe.py#L1522 https://github.com/MDAnalysis/mdanalysis/blob/5dc774784ba31ae04f18f9baf59ccfddd617fa74/package/MDAnalysis/core/universe.py#L1522 is in Merge() and only applies when building universes with the functionality. Is your problem related to the Merge() function and not about having cmaps in a Universe?

— Reply to this email directly, view it on GitHub https://github.com/MDAnalysis/mdanalysis/issues/3672#issuecomment-1133564120, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALSZECZHMBVD6H4JEDH74ULVLCM6VANCNFSM5WPZVXEQ. You are receiving this because you authored the thread.

ksy141 avatar May 21 '22 15:05 ksy141

I renamed the issue for the specific problem with Merge(). Chances for fixes are better if it's a small, well-characterized problem.

For the wider discussion for how to treat cmaps, it might be useful to start a discussion on the developer mailing list. Essentially you'd want to show developers that there's a general need to reconsider cmaps treatment. You'd want to explain what they are (not everybody knows them or uses them) and why it would be advantageous to have them treated like bonds, angles, etc. Let us know how you imagine this would work and what you would be able to do better/easier/more elegantly than before.

orbeckst avatar May 24 '22 17:05 orbeckst