search

COMCIFS / cif_core

The IUCr CIF core dictionary
15 stars 9 forks source link

cif_core.dic: categories `ATOM_ANALYTICAL_MASS_LOSS` and `ATOM_ANALYTICAL_SOURCE` should not have `ATOM_ANALYTICAL` as their parent #427

Closed vaitkus closed 1 year ago

vaitkus commented 1 year ago

The parent ATOM_ANALYTICAL and the children ATOM_ANALYTICAL_MASS_LOSS and ATOM_ANALYTICAL_SOURCE categories are all looped. The parent-child relationship for looped categories means that they could be joined on their key data names which must all be common, i.e. key items of the child category must be linked to the key items of the parent category. The previously mentioned categories do not seem to satisfy this requirement (nor do I think they should). Therefore, the ATOM_ANALYTICAL_MASS_LOSS and ATOM_ANALYTICAL_SOURCE should be assigned a different, non-looped parent category.

For more details on a similar topic see the following discussion: https://github.com/COMCIFS/cif_core/pull/371#discussion_r1236479775

jamesrhester commented 1 year ago

Yes, this is a definitely incorrect.

rowlesmr commented 1 year ago

I could have sworn I saw a PR that fixed this. But now I can't.

Is it as simple as changing ATOM_ANALYTICAL_MASS_LOSS and ATOM_ANALYTICAL_SOURCE to belong to ATOM?

vaitkus commented 1 year ago

There were similar PRs for other issues, but none for this specific one. It would probably be sufficient to place all those three categories (ATOM_ANALYTICAL, ATOM_ANALYTICAL_MASS_LOSS and ATOM_ANALYTICAL_SOURCE) under ATOM or we may introduce a new intermediate non-looped parent category which would hold all single-value parameters common to all the looped categories (if there are any such data items planned in the future). You are most familiar with the category, so I will be happy with whatever you and @jamesrhester decide on.

rowlesmr commented 1 year ago

We could go straight to ATOM, or maybe something like SPECIMEN or SPECIMEN_ANALYSIS as the intermediate.

jamesrhester commented 1 year ago

Let's just go straight to ATOM as a placeholder. If in the future we see the need to group these items together, we can then create the intermediate category with associated id data name. There is no impact on data file interpretation when changing categories in this way.

vaitkus commented 1 year ago

Ok, I set the parent category to ATOM.

I will implement the detection of such issues in the 'cif_ddlm_dic_check' script in the next few days.