COMCIFS / TopoCif

Development of the Topology CIF Dictionary
6 stars 6 forks source link

Data items _topol_node.fract_* should not be linked to the _atom_site.label data item #37

Open vaitkus opened 3 years ago

vaitkus commented 3 years ago

Data items _topol_node.fract_x, _topol_node.fract_y and _topol_node.fract_z are incorrectly linked to the _atom_site.label. The type, purpose and other attributes of the _topol_node.fract_* data items also seem incorrect (probably accidentally copied from _topol_node.atom_label). Based on the definition of the atom_site.fract_x data item, the definition bodies should be changed from:

    ...
    _name.linked_item_id          '_atom_site.label'
    _type.purpose                 Link
    _type.source                  Related
    _type.container               Single
    _type.contents                Code
    ...

to:

    ...
    _type.purpose                 Measurand
    _type.source                  Derived
    _type.container               Single
    _type.contents                Real
    _units.code                   none
    ...
vaitkus commented 3 years ago

Fixed by merging PR #38.

BobHanson commented 3 years ago

yes, and I see that the default is wrong there:

_enumeration.default = atom_site[topol_node.atom_label].fract_x

The default is the x component of (the atom_site fractional coordinate transformed by _topol_node.symop and _topol_node.translation).

How do we indicate that?

vaitkus commented 3 years ago

I totally missed the dREL code fragments. However, I do not yet feel knowledgeable enough to edit them anyway. Maybe @jamesrhester could help with this?

jamesrhester commented 3 years ago

The definition for _geom_bond.distance in the core dictionary does this calculation. There may however be no true default if not all topol_nodes have an atom_label, so perhaps better to just remove this.

BobHanson commented 3 years ago

good challenge. (leaving this for James) :)

On Mon, Sep 13, 2021 at 9:00 PM James Hester @.***> wrote:

The definition for _geom_bond.distance in the core dictionary does this calculation. There may however be no true default if not all topol_nodes have an atom_label, so perhaps better to just remove this.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/COMCIFS/TopoCif/issues/37#issuecomment-918730278, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEHNCW5PJTSS5BDEC7VM3NDUB2UEPANCNFSM5DGHNAIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Robert M. Hanson Professor of Chemistry St. Olaf College Northfield, MN http://www.stolaf.edu/people/hansonr

If nature does not answer first what we want, it is better to take what answer we get.

-- Josiah Willard Gibbs, Lecture XXX, Monday, February 5, 1900

We stand on the homelands of the Wahpekute Band of the Dakota Nation. We honor with gratitude the people who have stewarded the land throughout the generations and their ongoing contributions to this region. We acknowledge the ongoing injustices that we have committed against the Dakota Nation, and we wish to interrupt this legacy, beginning with acts of healing and honest storytelling about this place.

BobHanson commented 3 years ago

I have changed _type.source to Assigned and removed the method. Also adjusted the definitions. Close?

vaitkus commented 3 years ago

I think the issue is resolved and can be closed.

However, I am slightly unsure about the _type.source value change from Derived to Assigned since the _atom_site.fract_* data items currently also have the Derived value and the _topol_node.fract_* values will mostly likely be calculated from the initial atomic coordinates. Maybe @jamesrhester could offer some insight into the applicability of these _type.source values?

BobHanson commented 3 years ago

Yes, I thought you might notice that. _topolnode.fract* has no relation to ATOM SITE, it turns out!

On Wed, Sep 15, 2021 at 11:05 AM Antanas Vaitkus @.***> wrote:

I think the issue is resolved and can be closed.

However, I am slightly unsure about the _type.source value change from Derived to Assigned since the _atomsite.fract data items currently also have the Derived value and the _topolnode.fract values will mostly likely be calculated from the initial atomic coordinates. Maybe @jamesrhester https://github.com/jamesrhester could offer some insight into the applicability of these _type.source values?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/COMCIFS/TopoCif/issues/37#issuecomment-920154085, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEHNCW4LSTRRDYHR2LP4343UCC72ZANCNFSM5DGHNAIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Robert M. Hanson Professor of Chemistry St. Olaf College Northfield, MN http://www.stolaf.edu/people/hansonr

If nature does not answer first what we want, it is better to take what answer we get.

-- Josiah Willard Gibbs, Lecture XXX, Monday, February 5, 1900

We stand on the homelands of the Wahpekute Band of the Dakota Nation. We honor with gratitude the people who have stewarded the land throughout the generations and their ongoing contributions to this region. We acknowledge the ongoing injustices that we have committed against the Dakota Nation, and we wish to interrupt this legacy, beginning with acts of healing and honest storytelling about this place.

vaitkus commented 3 years ago

But is there really no relation even is the node consists of a single atom? The optional _topol_node.atom_label data item is linked to the _atom_site.label and the definition itself states that:

The atom label corresponding to this node and matching a value in
_atom_site.label when the node is associated with exactly one atom.
...

My (potentially incorrect) understanding was that this was done to indicate the original atom from the ATOM_SITE loop that served as a basis for the node.

BobHanson commented 3 years ago

Generally if there is a single atom, one would describe the atom using a reference to _atom_site.label and _topol_node.simop_id/translation. There would be no purpose in giving coordinates. It is allowable to give just the (transformed) coordinates or give both, but that would just be for convenience, the way a _geom_bond gives distances.

On Wed, Sep 15, 2021 at 2:58 PM Antanas Vaitkus @.***> wrote:

But is there really no relation even is the node consists of a single atom? The optional _topol_node.atom_label data item is linked to the _atom_site.label and the definition itself states that:

The atom label corresponding to this node and matching a value in _atom_site.label when the node is associated with exactly one atom. ...

My (potentially incorrect) understanding was that this was done to indicate the original atom from the ATOM_SITE loop that served as a basis for the node.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/COMCIFS/TopoCif/issues/37#issuecomment-920333612, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEHNCW6ZQBBLEBRYNCVEGLDUCD3GLANCNFSM5DGHNAIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Robert M. Hanson Professor of Chemistry St. Olaf College Northfield, MN http://www.stolaf.edu/people/hansonr

If nature does not answer first what we want, it is better to take what answer we get.

-- Josiah Willard Gibbs, Lecture XXX, Monday, February 5, 1900

We stand on the homelands of the Wahpekute Band of the Dakota Nation. We honor with gratitude the people who have stewarded the land throughout the generations and their ongoing contributions to this region. We acknowledge the ongoing injustices that we have committed against the Dakota Nation, and we wish to interrupt this legacy, beginning with acts of healing and honest storytelling about this place.

jamesrhester commented 2 years ago

Just to confirm, _type.source here should indeed by Derived, as either (i) the fract_{x,y,z} values are those of an atom or (ii) they have been calculated in some way from the atomic structure. I am aware that these data names could also be used to describe a completely abstract net, in which case everything would be strictly Assigned. For now I suggest we just opt for Derived on the assumption that the major use is with actual structures.

BobHanson commented 2 years ago

_topol_node.fract* are strictly for non-atomic nodes. They should not be used for atoms ever.

The data items _topol_node.fract_x, _topol_node.fract_y, and _topol_node.fract_z are symmetry-transformed fractional coordinates. They are needed only when the node refers to a non-atomic position that is not characterizable as a centroid, such as the center of a channel. If a node represents more than one atom from ATOM_SITE, fractional coordinates given in TOPOL_NODE give the author-preferred locus of the node for rendering purposes only. Otherwise, the centroid of those atoms is generally to be assumed.

I think originally we imagined them being potentially for atoms as well, but we dropped this option when we tightened up all the definitions relating to references to _atom_site. In the case that there are nodes that require this, we have been putting "." in for the _atom_site items that are coming from _topol_atom. See example_2.cif:

loop_ _topol_node.id _topol_node.label _topol_node.net_id _topol_node.fract_x _topol_node.fract_y _topol_node.fract_z 1 Li1 1 . . . # Li 2 C1 1 . . . # C 3 O1 1 . . . # O 4 Co1 1 . . . # Co 5 ZA1 2 . . . # Li 6 ZB1 2 0.25036 0.25036 0.25036 # (CO) 7 ZC1 2 . . . # Co

On Wed, Oct 20, 2021 at 8:31 PM James Hester @.***> wrote:

Just to confirm, type.source here should indeed by Derived, as either (i) the fract{x,y,z} values are those of an atom or (ii) they have been calculated in some way from the atomic structure. I am aware that these data names could also be used to describe a completely abstract net, in which case everything would be strictly Assigned. For now I suggest we just opt for Derived on the assumption that the major use is with actual structures.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/COMCIFS/TopoCif/issues/37#issuecomment-948177034, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEHNCW25YXBCRWOI36RX75DUH5UORANCNFSM5DGHNAIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Robert M. Hanson Professor of Chemistry St. Olaf College Northfield, MN http://www.stolaf.edu/people/hansonr

If nature does not answer first what we want, it is better to take what answer we get.

-- Josiah Willard Gibbs, Lecture XXX, Monday, February 5, 1900

We stand on the homelands of the Wahpekute Band of the Dakota Nation. We honor with gratitude the people who have stewarded the land throughout the generations and their ongoing contributions to this region. We acknowledge the ongoing injustices that we have committed against the Dakota Nation, and we wish to interrupt this legacy, beginning with acts of healing and honest storytelling about this place.

jamesrhester commented 2 years ago

Yes, I agree that the '.' is the perfect choice for these values in that example. I'm just aware that there is nothing ultimately stopping a user putting the exact same coordinates that the '.' imply, particularly those users that don't bother reading dictionaries. This is not a problem that needs solving, and none of which changes the Derived status of _type.source, which is all I was confirming.