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

Enumeration range for cell lengths #495

Closed
nautolycus opened this issue Jul 3, 2024 · 2 comments · Fixed by #497
Closed

Enumeration range for cell lengths #495

nautolycus opened this issue Jul 3, 2024 · 2 comments · Fixed by #497

Comments

@nautolycus
Copy link
Collaborator

Apologies if this has been considered before. The DDL1-based dictionary definition of the _cell_length_a etc. items has

_enumeration_range 0.0:

but the current core has e.g.

_enumeration.range 1.:

The enumeration range is imported from save_cell_length in templ_attr.cif and this seems to have entered at commit ada8718

I tend to favour the view that the enumeration ranges in the dictionary should be confined to values that are physically possible rather than what might be thought "reasonable" (leaving the latter to validation applications or overlays), so prefer the "non-negative" constraint rather than an arbitrary 1-angstrom cutoff. Any thoughts?

@vaitkus
Copy link
Collaborator

vaitkus commented Jul 3, 2024

@nautolycus this was indeed previously somewhat discussed in issue #64. The conclusion for the _cell_length_* items was to choose 1.0: as the new enumeration range based on the radius of a hydrogen atom and to backport this change to the legacy version of the DDL1 CIF_CORE dictionary (see https://github.com/COMCIFS/DDL1-legacy-dictionaries/blob/main/dictionaries/cif_core.dic where is change is already present since around 2017).

However, I think that the enumeration range can be changed (back) to 0.0: in both the DDL1 and the DDLm versions if needed without having a significant effect on the validation of existing CIF files. Personally, I also like the 0.0: enumeration range better, but maybe @jamesrhester knowns the original reasoning that led to this change when migrating from DDL1 to DDLm?

@jamesrhester
Copy link
Contributor

I have no insight to offer here, beyond supposing that perhaps certain dREL methods may have failed if provided with a zero-valued cell length, and so a minimal sensible value was chosen. I'm pretty certain there was no public discussion of this change, so reverting it would not be a problem. I have no objection to going back to 0.0.

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 a pull request may close this issue.

3 participants