COMCIFS / cif_core

The IUCr CIF core dictionary
15 stars 9 forks source link

Add `_exptl absorpt.coefficient_mu_su` #431

Closed rowlesmr closed 1 year ago

rowlesmr commented 1 year ago

Prompted by discussion https://github.com/COMCIFS/cif_core/pull/430#issuecomment-1605442143

_exptl_absorpt.coefficient_mu was defined as Number, but the description of this item is given in terms of values which are Measurands. _exptl_absorpt.coefficient_mu was changed to Derived Measurand, and the SU data item was added.

In the process, I noticed that a bunch of SU data items' _type.source was Related, which doesn't seem to link up with the description of Related, so I changed them all to match the _type.source of the data item to which the SU belongs.

rowlesmr commented 1 year ago

There is also at least one SU data item where the _type.source doesn't match the parent data item: _exptl_crystal.size_rad is Recorded, but _exptl_crystal.size_rad_su is Derived, but this is hidden through the general_su save frame.

Also, I didn't go further and edit in _import.get [{'file':templ_attr.cif 'save':general_su}] until I know that this is what you're after.

vaitkus commented 1 year ago

This should be split into two separate PRs -- one for the _exptl_absorpt.coefficient_mu_su and one for the _type.source changes -- so these issues could be discussed and merged independently.

Also, I think that the Related purpose might have been intentionally set for the SU items to show that they are related to the measurand data items. Not a 100% sure about that, but we should investigate a bit before making that change.

vaitkus commented 1 year ago

I checked the DDL1 dictionary and the _exptl_absorpt_coefficient_mu data item was also not allowed to have SU. Maybe there is a legitimate reason for that after all?

rowlesmr commented 1 year ago

Also, I think that the Related purpose might have been intentionally set for the SU items to show that they are related to the measurand data items. Not a 100% sure about that, but we should investigate a bit before making that change.

Then shouldn't every SU item be Related?

rowlesmr commented 1 year ago

I checked the DDL1 dictionary and the _exptl_absorpt_coefficient_mu data item was also not allowed to have SU. Maybe there is a legitimate reason for that after all?

The EXPTL category is

The CATEGORY of data items used to specify the experimental work prior to diffraction measurements. These include crystallization crystal measurements and absorption-correction techniques used.

_exptl_absorpt.coefficient_mu is

Absorption coefficient mu calculated from the atomic content of the cell, the density and the radiation wavelength.

So, the value of _exptl_absorpt.coefficient_mu should be calculated from things you know before the diffraction experiment. This ties in with it's Recorded status. It still doesn't mean that it shouldn't have an SU - the density, at least, would have an associated uncertainty. Being in EXPTL doesn't preclude having an SU; _exptl.transmission_factor_max, _exptl_crystal.density_meas, and many others have SUs.

.

Branching slightly out-of-scope: The use of EXPTL for "experimental work prior to diffraction measurements" is then muddied by _exptl_crystal.density_diffrn, where the dREL involves _cell.volume, which is a diffraction experiment thing. Although, there is _exptl_crystal.density_meas for "density measured using standard chemical and physical methods". _exptl_crystal.density_diffrn is the only data item (at all) to have "_diffrn" appended to it, which I'm guessing identifies it as a data item in the exptl category, which is actually made up of diffraction-related information.

Maybe related, is that _cell.atomic_mass doesn't have an SU, as the atomic mass is calculated from ATOM_TYPE loops, not ATOM_SITE. Which also means you can't report an atomic mass of a unit cell from a refined structure.

jamesrhester commented 1 year ago

Because the link from SU to the item it is the su of is specified via _name.linked_item_id, the _type.source is Related instead of derived. I note that this is not actually stated anywhere in ddl.dic, so perhaps we should rethink that or improve the Related description.

Regarding mu, I think it is entirely reasonable to allow it to have an su.

rowlesmr commented 1 year ago

Because the link from SU to the item it is the su of is specified via _name.linked_item_id, the _type.source is Related instead of derived. I note that this is not actually stated anywhere in ddl.dic, so perhaps we should rethink that or improve the Related description.

This is a wide ranging change. The general_su save frame gives everything _type.source Derived.

I am not qualified to properly parse the description of Related, but it does seem to only refer to looped lists.

# just FYI
         Related
;
         A value or tag used in the construction of looped lists of data.
         Typically identifying an item whose unique value is the reference key
         for a loop category and/or an item which has values in common with
         those of another loop category and is considered a Link between these
         lists.
;