Skip to content

Add structure keys to _atom_site_*#44

Closed
rowlesmr wants to merge 3 commits into
COMCIFS:mainfrom
rowlesmr:add-structure-keys
Closed

Add structure keys to _atom_site_*#44
rowlesmr wants to merge 3 commits into
COMCIFS:mainfrom
rowlesmr:add-structure-keys

Conversation

@rowlesmr

@rowlesmr rowlesmr commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

close #43, see also #41

ATOM_SITE gained an extra key in multi_block_core. This PR adds a linked structure dataname to each category referencing ATOM_SITE.

@rowlesmr rowlesmr changed the title Add structure keys Add structure keys to _atom_site_* Jun 15, 2026
@rowlesmr

rowlesmr commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

Should ATOM_SITE_FOURIER_WAVE_VECTOR get a structure id?

I need to fully investigate how it interacts with CELL_WAVE_VECTOR

@rowlesmr

Copy link
Copy Markdown
Contributor Author

Arg! Pretty sure that the following categories should have *.structure_id as a category key. The category descriptions talk about describing features in a modulated structure, so should be a key.

        ATOM_SITE_ANHARMONIC_ADP_FOURIER
        ATOM_SITE_ANHARMONIC_ADP_ORTHO
        ATOM_SITE_ANHARMONIC_ADP_XHARM
        ATOM_SITE_DISPLACE_FOURIER
        ATOM_SITE_DISPLACE_LEGENDRE
        ATOM_SITE_DISPLACE_ORTHO
        ATOM_SITE_DISPLACE_XHARM
        ATOM_SITE_OCC_FOURIER
        ATOM_SITE_OCC_LEGENDRE
        ATOM_SITE_OCC_ORTHO
        ATOM_SITE_OCC_XHARM
        ATOM_SITE_ROT_FOURIER
        ATOM_SITE_ROT_LEGENDRE
        ATOM_SITE_ROT_ORTHO
        ATOM_SITE_ROT_XHARM
        ATOM_SITE_U_FOURIER
        ATOM_SITE_U_LEGENDRE
        ATOM_SITE_U_ORTHO
        ATOM_SITE_U_XHARM

@rowlesmr

rowlesmr commented Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Fails semantic tests for:

/github/workspace/cod-tools/scripts/cif_ddlm_dic_check: ./cif_ms.dic data_CIF_MS save_ATOM_SITE_ANHARMONIC_ADP_FOURIER_PARAM: WARNING, the '_atom_site_anharmonic_adp_fourier_param.structure_id' key item is linked to the '_structure.id' data item that is not one of the key items of the 'ATOM_SITE_ANHARMONIC_ADP_FOURIER' parent category -- the key item sets of two looped categories that are bound by a child-parent relationship must have a formal one-to-one correspondence.
/github/workspace/cod-tools/scripts/cif_ddlm_dic_check: ./cif_ms.dic data_CIF_MS save_ATOM_SITE_DISPLACE_FOURIER_PARAM: WARNING, the '_atom_site_displace_fourier_param.structure_id' key item is linked to the '_structure.id' data item that is not one of the key items of the 'ATOM_SITE_DISPLACE_FOURIER' parent category -- the key item sets of two looped categories that are bound by a child-parent relationship must have a formal one-to-one correspondence.
/github/workspace/cod-tools/scripts/cif_ddlm_dic_check: ./cif_ms.dic data_CIF_MS save_ATOM_SITE_OCC_FOURIER_PARAM: WARNING, the '_atom_site_occ_fourier_param.structure_id' key item is linked to the '_structure.id' data item that is not one of the key items of the 'ATOM_SITE_OCC_FOURIER' parent category -- the key item sets of two looped categories that are bound by a child-parent relationship must have a formal one-to-one correspondence.
/github/workspace/cod-tools/scripts/cif_ddlm_dic_check: ./cif_ms.dic data_CIF_MS save_ATOM_SITE_ROT_FOURIER_PARAM: WARNING, the '_atom_site_rot_fourier_param.structure_id' key item is linked to the '_structure.id' data item that is not one of the key items of the 'ATOM_SITE_ROT_FOURIER' parent category -- the key item sets of two looped categories that are bound by a child-parent relationship must have a formal one-to-one correspondence.
/github/workspace/cod-tools/scripts/cif_ddlm_dic_check: ./cif_ms.dic data_CIF_MS save_ATOM_SITE_U_FOURIER_PARAM: WARNING, the '_atom_site_u_fourier_param.structure_id' key item is linked to the '_structure.id' data item that is not one of the key items of the 'ATOM_SITE_U_FOURIER' parent category -- the key item sets of two looped categories that are bound by a child-parent relationship must have a formal one-to-one correspondence.

The errors are all of the same thing. Explaining ATOM_SITE_U_FOURIER:

The keys of ATOM_SITE_U_FOURIER are _atom_site_U_Fourier.id (opaque identifer) and _atom_site_U_Fourier.structure_id (linked to _structure.id)

The keys of ATOM_SITE_U_FOURIER_PARAM (a child category of ATOM_SITE_U_FOURIER) are _atom_site_U_Fourier_param.id (linked to _atom_site_U_Fourier.id) and _atom_site_U_Fourier_param.structure_id (linked to _structure.id)

The error is there because _atom_site_U_Fourier_param.structure_id is not linked to a member of ATOM_SITE_U_FOURIER.

Do the *fourier_param.structure_id links need to be changed to *fourier.structure_id?, or is the check being hyper-sensitive.

@jamesrhester

Copy link
Copy Markdown
Contributor

Do the *fourier_param.structure_id links need to be changed to *fourier.structure_id?, or is the check being hyper-sensitive.

I know that e.g. the mmCIF/DDL2 world consider all linked data names to be "the same thing" and go as far as listing all of the linked child data names in the parent definition. So it is open to us to just say that a common ancestor linked name is sufficient for two data names to be the same.

The more DDL1-ish way of looking at it is that the child data name is restricted to the values taken by the parent, so you could have a smaller and smaller subset of possible values as you go down the linked data name hierarchy. I think that is unnecessarily restrictive and hierarchical thinking tends to be pointlessly restrictive in relational contexts. It also won't work in pdCIF for the PD_DATA category, where we suppose that the full list of point IDs is the amalgamation of all point IDs from the child categories, rather than the child categories being restricted to whatever is explicitly available in the parent category.

This check passes for multiblock because we do as @rowlesmr proposes and link to the relevant data name in the parent category rather than the top level data name. So that's a way to get around the check if you want to.

@vaitkus , could we perhaps modify that check to check for a common ancestor data name?

Added notes about omitting structure_id for various ATOM_SITE categories
@vaitkus

vaitkus commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

The more DDL1-ish way of looking at it is that the child data name is restricted to the values taken by the parent, so you could have a smaller and smaller subset of possible values as you go down the linked data name hierarchy. I think that is unnecessarily restrictive and hierarchical thinking tends to be pointlessly restrictive in relational contexts. It also won't work in pdCIF for the PD_DATA category, where we suppose that the full list of point IDs is the amalgamation of all point IDs from the child categories, rather than the child categories being restricted to whatever is explicitly available in the parent category.

I must admit that I quite like the hierarchical approach as it allows to more easily detect and diagnose certain data discrepancies, e.g. situations where the ATOM_SITE_U_FOURIER loop refers to a structure that does not even have an ATOM_SITE loop.

This check passes for multiblock because we do as @rowlesmr proposes and link to the relevant data name in the parent category rather than the top level data name. So that's a way to get around the check if you want to.

I would say that would be the way to go, at least for now.

@vaitkus , could we perhaps modify that check to check for a common ancestor data name?

Allowing common ancestry instead of direct ancestry seems like an extension of the current interpretation of parent-child looped categories. That being said, I will code the checks to whichever approach is agreed upon. Currently, I based the logic on the following fragment from a draft version of one of the ITG chapters [1]:

No data names are contained within a Head category, and all ’Set’ categories must be
children of this ’Head’ category. ’Loop’ categories may be children of either the ’Head’
category, or of another ’Loop’ category. In the latter case the two categories can in
principle be interpreted as a single ’Loop’ category. The key data names of the parent
category each correspond to a key data name in the child category, and the values
of either set of key data names can be used for the purpose of identifying rows in
either category. A data file may present data from such categories either in a single
table, or as two separate tables. In relational database terms, the merged category
is the result of a left outer join of the parent category with the child category using
the key data names.

Maybe there are other chapters that specify the nature of the looped child-parent
categories and explicitly allow common ancestral relationships between keys,
however, I would like at least one such reference. Unfortunately, the DDLm
reference dictionary is silent on this. An explicit approval of ancestral
relationships in this discussion thread would also work, although I falls
more into the grey literature category.

[1] Construction and interpretation of CIF dictionaries, Section Loop and Set categories.

@rowlesmr

Copy link
Copy Markdown
Contributor Author

I am not bothered by any outcome, and can see the good of both approaches. I was just suprised coming from the pd background

I think drawing from a smaller pool of possible values is a good reason to keep it that way 🤷

@rowlesmr

rowlesmr commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

This PR has managed to get stuffed around. Easier to start again.

@rowlesmr rowlesmr closed this Jul 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Insufficient keys specified re _atom_site.label

3 participants