Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add units to templ_enum.cif #396

Merged
merged 7 commits into from
Jun 28, 2023
Merged

Conversation

vaitkus
Copy link
Collaborator

@vaitkus vaitkus commented May 15, 2023

This PR serves as an example for issue #391.

but questions still remain
@rowlesmr
Copy link
Collaborator

rowlesmr commented Jun 3, 2023

Just exploring this idea. Found some issues around units.

@vaitkus vaitkus marked this pull request as ready for review June 26, 2023 12:16
@vaitkus
Copy link
Collaborator Author

vaitkus commented Jun 26, 2023

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?

@vaitkus vaitkus linked an issue Jun 26, 2023 that may be closed by this pull request
@rowlesmr
Copy link
Collaborator

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
Copy link
Contributor

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.

@vaitkus vaitkus changed the title An example of adding units to templ_enum.cif save frames Add units to templ_enum.cif Jun 28, 2023
@vaitkus vaitkus merged commit c136047 into COMCIFS:master Jun 28, 2023
3 checks passed
@vaitkus vaitkus deleted the add-units-to-temp-enum branch June 28, 2023 11:15
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.

templ_enum.cif: add _units.code to save frames
3 participants