COMCIFS / magnetic_dic

Development of the magnetic CIF dictionary
0 stars 4 forks source link

Update symmform examples #68

Closed vaitkus closed 6 months ago

vaitkus commented 6 months ago

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.

vaitkus commented 6 months ago

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 https://github.com/COMCIFS/magnetic_dic/issues/67#issuecomment-1928842478).

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] https://github.com/COMCIFS/magnetic_dic/issues/67#issuecomment-1928836639

brantonc commented 6 months ago

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)https://github.com/COMCIFS/magnetic_dic/issues/67#issuecomment-1928842478).

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)https://github.com/COMCIFS/magnetic_dic/issues/67#issuecomment-1928836639

— Reply to this email directly, view it on GitHubhttps://github.com/COMCIFS/magnetic_dic/pull/68#issuecomment-1930351537, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACZAIQRPYETKSUI3NAXKA6DYSJNX3AVCNFSM6AAAAABC2JTTXWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZQGM2TCNJTG4. You are receiving this because you are subscribed to this thread.Message ID: @.**@.>>

vaitkus commented 6 months ago

Thank you for the review.