richardjgowers
commented
2 years ago A non breaking change is to duplicate the attribute, which would actually require a different attribute annoyingly (one which defines two attributes to supply)
Open IAlibay opened 2 years ago
richardjgowers
commented
2 years ago A non breaking change is to duplicate the attribute, which would actually require a different attribute annoyingly (one which defines two attributes to supply)
IAlibay
commented
2 years ago I was originally thinking we could do the old bfactor trick for warnings,
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.
richardjgowers
commented
2 years ago 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,
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: @.***>
Expected behavior
The contents of
chargesis 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
FormalChargeinstead.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.