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

Symmetry operation specifications in Jones faithful notation (defined by PCRE) #464

Merged
merged 110 commits into from
Dec 19, 2023

Conversation

sauliusg
Copy link
Contributor

@sauliusg sauliusg commented Jun 6, 2023

Please have a look at the symmetry operation specification PR with the Jones' faithful notation of the symmetry operations.

Word description was added on top of formal PCRE definitions of the syntax.

To specify the semantics (interpretation) of the symmetry operations, I had to cite very specific places of the ITC vols. A and B. As a result, there are many references in the text now.

For now, I put the references after each definition section; this however results in scattered bibliography list and duplications (one duplication is present even now). I think it would be better to make a separate "Bibliography" section at the end of the text and place all bibliography sources there, with just references inserted into the text, using the Harvard referencing style (Author-year). If you agree (or are not much against this suggestion), I can rearrange references in this or in a separate PR.

I had to update also the definition of the Hall symbol; to be machine-parsable it needs to be more specific and allow the change-of-basis operations (COD has such entries).

There is currently no discussion on the rationale and intended use of H-M and Hall symbols. Basically the short Hall symbols are in 1:1 correspondence with the ITC space group numbers and fail to provide the setting and the cell choice information. The benefit of the H-M symbols however is that they are much more human-readable and allow to understand the properties of the space group without even looking into the ITC (unlike the pure ITC numbers). Good for searches.

The full (extended) H-M symbols, as well as Hall symbols (both with the c-o-b operations specified) should permit unique identification of the symmetry operation list (Hall symbols after algorithmic interpretation, H-M symbols after table lookup). If you think that this explanation would be needed in the definition text, please let me know, I'll add it.

@sauliusg
Copy link
Contributor Author

sauliusg commented Jun 6, 2023

NB: the text will be added/expanded later, for now its just the REs and the tests.

merkys
merkys previously requested changes Jun 6, 2023
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

The regular expression for symmetry operators looks good to me. Text tying them to OPTIMADE properties is needed, but the regex part seems appropriate.

optimade.rst Show resolved Hide resolved
optimade.rst Show resolved Hide resolved
optimade.rst Show resolved Hide resolved
@rartino
Copy link
Contributor

rartino commented Jun 9, 2023

Workshop suggestion: we accept the syntax specification. The remaining fields from #416 #416 (comment) should be added to the PR and time given to symmetry description stakeholders to review. @rartino @sauliusg @vaitkus @merkys @giovannipizzi (feel free to post a comment below to add yourself to the stakeholders)

@rartino rartino added the PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR label Jun 11, 2023
@sauliusg
Copy link
Contributor Author

sauliusg commented Nov 3, 2023

That being said, I do not think that moving the mention of fractional coordinates to an example helps, because we are still mentioning a property that is not currently supported.

To make it clear – after the rephrasing the text no longer refers to any property, either existing or not. The "fractional atom coordinates" expression that is used in the definition refers to a general crystallographic values. If there is a property in OPTIMADE that returns these values, the property can be used. If there is no such property, the fractional coordinates need to be computer in one way or another, and it is up to the client to do so.

I've made this change in the text on purpose, so that it remains correct regardless whether OPTIMADE defines a fractional coordinate property or not. On this way we can merge this PR now and will not have to return to it when the fractional coordinate property is actually defined.

To clarify the definition, I have added an example on how to deal with the situation when only Cartesian coordinates are returned in a response (see the sentence cited below).

Also, I found the earlier phrasing more succinct and easier to understand -- now the reader if left to wonder what are those different coordinate forms to which the symmetry operations can be applied.

In this case the reader is referred to standard textbooks on crystallography. Should we give a reference?

In your earlier comment you mention Cartesian coordinates as one such case,

I cannot find the place where I mention application of symmetry operations to Cartesian coordinates; that would be also incorrect. Could you please give me the exact location of the text that you cite?

but that would clash with other statements presented in this PR, e.g.:

The symmetry operations are to be applied to fractional atom coordinates.

I can not find any clash here; as said, I can not find the place where Cartesian coordinates are mentioned as suitable for symop applications. Thus there should be no clash or contradiction here.

I have added an explanation of how Cartesian coords could be used with symmetry operations: "In case only Cartesian coordinates are available, these Cartesian coordinates must be converted to fractional coordinates before application of the provided symmetry operations." . How about that?

Finally, if the new phrasing is accepted, the can and must part should probably be rewritten to use either "can" or "must", because in this situation "must" encompasses "can". I would probably go with "can".

I disagree. The symops as given can be applied to fractional coordinates. They must be applied if fractional coordinates are given for the AU only and the user wants to reconstruct the full unit cell. Both cases are relevant.

@vaitkus
Copy link
Contributor

vaitkus commented Nov 3, 2023

@sauliusg wrote:

I cannot find the place where I mention application of symmetry operations to Cartesian coordinates; that would be also incorrect. Could you please give me the exact location of the text that you cite?

I was referring to part of the comment from (#464 (comment)):

Actually, symmetry operations can also be applied to atoms when Cartesian coordinates are returned; in that case one needs to convert them to fractional coords (which is possible since the unit cell vectors are also provided), apply the symmetry operations and then convert back to Cartesian if needed.

But this has now been addressed with an additional sentence in the PR.

I disagree. The symops as given can be applied to fractional coordinates. They must be applied if fractional coordinates are given for the AU only and the user wants to reconstruct the full unit cell. Both cases are relevant.

Then it should be "which these operations can or must be applied" instead of "which these operations can and must be applied" ("and" -> "or").

@sauliusg
Copy link
Contributor Author

sauliusg commented Nov 3, 2023

Actually, symmetry operations can also be applied to atoms when Cartesian coordinates are returned; in that case one needs to convert them to fractional coords (which is possible since the unit cell vectors are also provided), apply the symmetry operations and then convert back to Cartesian if needed.

But this has now been addressed with an additional sentence in the PR.

OK, so this is hopefully resolved now.

I disagree. The symops as given can be applied to fractional coordinates. They must be applied if fractional coordinates are given for the AU only and the user wants to reconstruct the full unit cell. Both cases are relevant.

Then it should be "which these operations can or must be applied" instead of "which these operations can and must be applied" ("and" -> "or").

Good suggestion, thanks, I will apply it!

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
…be applied" action is mentioned in the revious sentence... accepted.

Co-authored-by: Antanas Vaitkus <antanas.vaitkus90@gmail.com>
@sauliusg sauliusg requested a review from vaitkus November 3, 2023 16:13
@ml-evs ml-evs added topic/property-standardization The specification of the precise data representation of properties and entries status/has-concrete-suggestion This issue has one or more concrete suggestions spelled out that can be brought up for consensus. blocking-release This is a PR or issue that presently blocks the release of next version of the spec. labels Dec 19, 2023
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Comments during meeting.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

One more edit from meeting.

optimade.rst Outdated Show resolved Hide resolved
sauliusg and others added 4 commits December 19, 2023 16:21
Co-authored-by: Rickard Armiento <gitcommits@armiento.net>
Co-authored-by: Rickard Armiento <gitcommits@armiento.net>
Applying suggestions discussed in the meeting.

Co-authored-by: Rickard Armiento <gitcommits@armiento.net>
Copy link
Contributor

@rartino rartino 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 now!

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks for the hard work!

@sauliusg sauliusg merged commit df152e9 into Materials-Consortia:develop Dec 19, 2023
5 checks passed
@blokhin
Copy link
Member

blokhin commented Dec 20, 2023

Let me also add a direct link to the CIF dictionary mentioned. Also I’d like to check, if the ops non-symbolic notation is discouraged (e.g. x+0.6666667 should be x+2/3).

@vaitkus
Copy link
Contributor

vaitkus commented Jan 12, 2024

@blokhin wrote:

Let me also add a direct link to the CIF dictionary mentioned. Also I’d like to check, if the ops non-symbolic notation is discouraged (e.g. x+0.6666667 should be x+2/3).

A link to the CIF_CORE dictionary is provided under the space_group_symmetry_operations_xyz section as:

https://www.iucr.org/__data/iucr/cifdic_html/1/cif_core.dic/Ispace_group_symop_operation_xyz.html

The non-symbolic notation is not allowed at all by the regular expression, that is, x+2/3 is allowed, x+0.6666667 is not allowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-release This is a PR or issue that presently blocks the release of next version of the spec. status/has-concrete-suggestion This issue has one or more concrete suggestions spelled out that can be brought up for consensus. topic/property-standardization The specification of the precise data representation of properties and entries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants