search

COMCIFS / cif_core

The IUCr CIF core dictionary
15 stars 9 forks source link

Add units to templ_enum.cif #396

Closed vaitkus closed 1 year ago

vaitkus commented 1 year ago

This PR serves as an example for issue #391.

rowlesmr commented 1 year ago

Just exploring this idea. Found some issues around units.

vaitkus commented 1 year ago

One thing that we need to decide on before merging is if we want to only have they units in the templ_enum.cif file or also in the cif_core.dic dictionary file. The two-place approach may be more readable, but it also violates the single point of truth principle. In other words, at some point we may update the units in the dictionary file, but forget to update them in the templ_enum.cif. This mismatch is quite easy to miss, since the contents are imported with the replace-on-duplicate mode which silently prefers the data items from the imported source.

@jamesrhester , @rowlesmr do you have any opinion on the topic?

rowlesmr commented 1 year ago

Having units in both places maximises readbility, but sacrifices robustness.

How is the documentation generated? ie, will the units_code explicitly given in Vol G / the website (eg https://www.iucr.org/__data/iucr/cifdic_html/1/cif_core.dic/Iatom_type_scat_length_neutron.html)

I'm assuming it is A Big Deal to change from replace-on-duplicate?

jamesrhester commented 1 year ago

I strongly believe the units should only be in one place.

A very recent decision is that the released dictionary as it appears on the IUCr website will have all of the mode=Contents imports resolved in place, so the readability should not suffer. I am planning that the release files that Github lists will also have these resolved, but haven't got around to figuring out how that works in Github yet.

I think that means that moving the units to the enum file is a good idea, as it reduces the length of the main file.