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

Added templates for modulated structure dictionary. #508

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

jamesrhester
Copy link
Contributor

These additional template blocks supplied by G Madariaga as part of modulated structure dictionary update.

Copy link
Collaborator

@vaitkus vaitkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall.

However, additional semantic incompatibilities may become evident once these items are put to use in actual import statements.

templ_attr.cif Outdated Show resolved Hide resolved
templ_attr.cif Show resolved Hide resolved
templ_attr.cif Show resolved Hide resolved
templ_attr.cif Show resolved Hide resolved
templ_attr.cif Show resolved Hide resolved
templ_attr.cif Outdated Show resolved Hide resolved
templ_attr.cif Outdated Show resolved Hide resolved
_description_example.detail
'.' 'no symmetry or translation to site'
'4' '4th symmetry operation applied'
'7_645' '7th symmetry position; +a on x, -b on y'
Copy link
Collaborator

@vaitkus vaitkus Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The '7_645' example may be incompatible with the provided definition since the translation part consists of only 3 elements (p = 3).

The definition states that:

The number 'p' must agree with (_cell_modulation_dimension + 3). 

The _cell_modulation_dimension is currently defined to have values in the range of 1..8, thus leaving the minimal p value at 4.

Also, having an example with at least 4 translations would be useful to showcases the difference from the conventional 3D symmetry code data item.

superspace coordinates. It must match a number given in
_space_group_symop_ssg_id. 'm1...mp' refer to the translations
that are subsequently applied to the symmetry-transformed
coordinates to generate the atom used in calculating the angle.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "used in calculating the angle" part here is probably a bit too specific. I assume that this save frame will also be imported by items that do not deal with angles, e.g. _geom_bond.site_ssg_symmetry_1.

jamesrhester and others added 7 commits December 13, 2024 13:31
Co-authored-by: Antanas Vaitkus <[email protected]>
Co-authored-by: Antanas Vaitkus <[email protected]>
Co-authored-by: Antanas Vaitkus <[email protected]>
Co-authored-by: Antanas Vaitkus <[email protected]>
Co-authored-by: Antanas Vaitkus <[email protected]>
Co-authored-by: Antanas Vaitkus <[email protected]>
Co-authored-by: Antanas Vaitkus <[email protected]>
@jamesrhester
Copy link
Contributor Author

However, additional semantic incompatibilities may become evident once these items are put to use in actual import statements.

The corresponding dictionary updates are now available as a pull request.

loop_
_description_example.case
_description_example.detail
'.' 'no symmetry or translation to site'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'.' 'no symmetry or translation to site'
. 'no symmetry or translation to site'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The special value should not be quoted to distinguish it from a full stop. The same approach is taken in the 'site_symmetry' save frame.

_type.source Related
_type.container Single
_type.contents Integer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_enumeration.range 0:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The '_cell_wave_vector.seq_id' is defined as having the enumeration range of [0. +inf) therefore the same should extend to the linked items.

P.S.: Judging from the usage examples the enumeration range should most likely be [1; +inf) for all of these items, but this point can be addressed in a separate issue.

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.

2 participants