mdanalysis
mdanalysis copied to clipboard
MDAnalysis
Change mmtf reading of formal charges from the `charges` attribute to `formalcharges`
Expected behavior
The contents of charges is always expected to contain partial charges.
Actual behavior
For mmtf, formal charges are fed into charges, which a) leads to confusing attribute contents, b) now differs from what the PDB reader does.
Code to reproduce the behavior
See https://github.com/MDAnalysis/mdanalysis/blob/d5c7cb9a7807b06c169b418dd5c6c072c33580f5/package/MDAnalysis/topology/MMTFParser.py#L188
Proposal
Switch to using FormalCharge instead.
That being said, this is possibly a breaking changing for 2.x? I see it as a bug but most folks probably don't. I'm not really sure how we would suitably warn users about the change though.
A non breaking change is to duplicate the attribute, which would actually require a different attribute annoyingly (one which defines two attributes to supply)
I was originally thinking we could do the old bfactor trick for warnings,
https://github.com/MDAnalysis/mdanalysis/blob/d5c7cb9a7807b06c169b418dd5c6c072c33580f5/package/MDAnalysis/core/topologyattrs.py#L361-L366
But I'm not sure we can suitably pass across the reader class to TopologyAttrs to specifically do that when it's the MMTF reader.
Yeah I’m not against nasty hacks for this tbh…
On Sat, Jul 23, 2022 at 17:08, Irfan Alibay @.***> wrote:
I was originally thinking we could do the old bfactor trick for warnings,
https://github.com/MDAnalysis/mdanalysis/blob/d5c7cb9a7807b06c169b418dd5c6c072c33580f5/package/MDAnalysis/core/topologyattrs.py#L361-L366
But I'm not sure we can suitably pass across the reader class to TopologyAttrs to specifically do that when it's the MMTF reader.
— Reply to this email directly, view it on GitHub https://github.com/MDAnalysis/mdanalysis/issues/3762#issuecomment-1193148427, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACGSGB75UKX63JKH5I3422TVVQKG7ANCNFSM54OGCGPQ . You are receiving this because you commented.Message ID: @.***>