Update symmform examples #68
Conversation
…FS#67. Specifically, examples were added for the `_atom_site_moment_Fourier_param.modulus_symmform`, `_atom_site_moment_Fourier_param.phase_symmform` and `_atom_site_moment_Fourier_param_sin_symmform` data items.
The previous example was replaced by examples provided in issue COMCIFS#67. The previous value was merge with the example of the ATOM_SITE_MOMENT_FOURIER_PARAM category. The category example was only slightly modified by replacing the dotless data names with their dotted counterparts and correcting several symmform values that lacked a trailing modulation number.
…OMCIFS#67. The symmform symbolic expression description part was copied from the definition of `_atom_site_moment_Fourier_param.cos_symmform` and adapted to better reflect the syntax of each individual item.
As specified in COMCIFS#67 (comment), the 4th character is a numeric code as specified in the _atom_site_moment_Fourier.wave_vector_seq_id data item.
|
This PR became slightly more complex than I originally expected, but hopefully it is still granular enough to review (and potentially merge). If not, we can postpone it to the next dictionary release. This PR now rewrites the descriptions of 4 symmform data items [1] to better reflect the syntax which applies for each individual data item. This was done by copying the description from Furthermore, it was noted in each description that the 4th character which identifies the modulation vector is actually taken from Finally, examples for each of these data items were also added. I think I applied all of the suggestions correctly, but I strongly suggest a mandatory review before merging. [1] |
|
After this round of merges, I’m happy to comb through the updated version in search of other problems.
From: Antanas Vaitkus ***@***.***>
Sent: Tuesday, February 6, 2024 9:53 AM
To: COMCIFS/magnetic_dic ***@***.***>
Cc: Subscribed ***@***.***>
Subject: Re: [COMCIFS/magnetic_dic] Update symmform examples (PR #68)
This PR became slightly more complex than I originally expected, but hopefully it is still granular enough to review (and potentially merge). If not, we can postpone it to the next dictionary release.
This PR now rewrites the descriptions of 4 symmform data items [1] to better reflect the syntax which applies for each individual data item. This was done by copying the description from _atom_site_moment_Fourier_param.cos_symmform, merging it with the description provided in [2] and then further modifying the symbol syntax description. For example, the 3rd character in the symbols of _atom_site_moment_Fourier_param.phase_symmform was restricted from [s,c,m,p] (sin, cos, modulated, phase) to only [p] (phase).
Furthermore, it was noted in each description that the 4th character which identifies the modulation vector is actually taken from _atom_site_moment_Fourier.wave_vector_seq_id (as stated in #67 (comment)<#67 (comment)>).
Finally, examples for each of these data items were also added.
I think I applied all of the suggestions correctly, but I strongly suggest a mandatory review before merging.
[1] _atom_site_moment_Fourier_param.sin_symmform, _atom_site_moment_Fourier_param.cos_symmform, _atom_site_moment_Fourier_param.modulus__symmform, _atom_site_moment_Fourier_param.phase_symmform.
[2] #67 (comment)<#67 (comment)>
—
Reply to this email directly, view it on GitHub<#68 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACZAIQRPYETKSUI3NAXKA6DYSJNX3AVCNFSM6AAAAABC2JTTXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZQGM2TCNJTG4>.
You are receiving this because you are subscribed to this thread.Message ID: ***@***.******@***.***>>
|
There was a problem hiding this comment.
Approving for merging, @brantonc can review after merging if that is preferred.
|
Thank you for the review. |
This PR aims at resolving issue #67.
This PR is currently marked as a draft since there are still a few minor issues with the symmform definitions that should be resolved.