-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RO-4209: Update polymer instance features with ligand interaction enumerations #140
Conversation
…es; Add connect_type to _rcsb_ligand_neighbors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @piehld. Please see my comments below.
_item_enumeration.detail | ||
'covalent bond' 'The target is bound through a covalent interaction' | ||
'metal coordination' 'The target is bound through a metal-coordination interaction' | ||
'non-bonded' 'The target is not bound through a covalent or metal-coordination interaction' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the value here be non-bonded interaction
rather than just non-bonded
, which can be misunderstood as no interaction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that could work, but I hesitate since "interaction" isn't included in the other enumeration values. What are your thoughts, @valasatava?
@@ -208,6 +213,9 @@ save__rcsb_entity_instance_feature.type | |||
'ZERO_OCCUPANCY_ATOM_XYZ' "Atom coordinates with zero occupancy" 'Zero Occupancy Atoms' . | |||
'UNOBSERVED_ATOM_XYZ' "Unobserved atom coordinates" 'Unobserved Atoms' . | |||
'UNOBSERVED_RESIDUE_XYZ' "Unobserved residue coordinates" 'Unobserved Residues' . | |||
'LIGAND_INTERACTION' 'Ligand interactions with macromolecular target' 'Ligand Interactions' . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does LIGAND_INTERACTION
correspond to non-bonded interactions only or does this include all types of interactions? Can this be clarified in the dictionary definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It includes all types of interactions. I'll update the description to convey that.
_item_enumeration.detail | ||
'covalent bond' 'The target is bound through a covalent interaction' | ||
'metal coordination' 'The target is bound through a metal-coordination interaction' | ||
'non-bonded' 'The target is not bound through a covalent or metal-coordination interaction' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the values of _item_enumeration.detail
should be The ligand is bound through...
instead of The target is bound through...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that, will update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @piehld. Looks good.
RO-4209