search

COMCIFS / magnetic_dic

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

Fix `*_transform_pp_abc` items #64

Closed vaitkus closed 7 months ago

vaitkus commented 7 months ago

This PR mainly addresses inconsistencies in the definitions of the _parent_space_group.child_transform_pp_abc and _parent_space_group.transform_pp_abc data items. Specifically, the attributes assigned to these items did not seem compatible with their human-readable definitions. I tried to fix it using similar data items (e.g. _space_group_magn.transform_BNS_Pp_abc) as a template.

Also, looking at other similar *_pp_abc items I noticed that they are often also accompanied by the *_pp item which records the transformation matrix. Maybe that was also the original intention with these items and the corresponding _parent_space_group.child_transform_pp and _parent_space_group.transform_pp items should be added?

Finally, I also changed the purpose of several other *_pp_abc items from Describe to Encode since I assume that they are intended to be machine-readable (similarly to the _space_group_symop.operation_xyz data item from CIF_CORE). Note, that _space_group_magn_transforms.pp_abc was already assigned the Encode purpose.

brantonc commented 7 months ago

I agree that the _parent_space_group category needed work. These changes seem reasonable. Adding the corresponding _Pp items is a good idea. Thank you. Note that these tags are used extensively by Manu in the MAGNDATA database of magnetic structures on the Bilbao Crystallographic server. See any example there. I include Manu here perchance he is not on these emails by default. https://www.cryst.ehu.es/magndata/index.php?show_db=1

Branton

From: Antanas Vaitkus @.> Sent: Friday, January 19, 2024 9:19 AM To: COMCIFS/magnetic_dic @.> Cc: Subscribed @.**> Subject: [COMCIFS/magnetic_dic] Fix `_transform_pp_abc` items (PR #64)

This PR mainly addresses inconsistencies in the definitions of the _parent_space_group.child_transform_pp_abc and _parent_space_group.transform_pp_abc data items. Specifically, the attributes assigned to these items did not seem compatible with their human-readable definitions. I tried to fix it using similar data items (e.g. _space_group_magn.transform_BNS_Pp_abc) as a template.

Also, looking at other similar _pp_abc items I noticed that they are often also accompanied by the _pp item which records the transformation matrix. Maybe that was also the original intention with these items and the corresponding _parent_space_group.child_transform_pp and _parent_space_group.transform_pp items should be added?

Finally, I also changed the purpose of several other *_pp_abc items from Describe to Encode since I assume that they are intended to be machine-readable (similarly to the _space_group_symop.operation_xyz data item from CIF_CORE). Note, that _space_group_magn_transforms.pp_abc was already assigned the Encode purpose.

You can view, comment on, or merge this pull request online at:

https://github.com/COMCIFS/magnetic_dic/pull/64

Commit Summary

File Changes

(1 filehttps://github.com/COMCIFS/magnetic_dic/pull/64/files)

Patch Links:

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

vaitkus commented 7 months ago

Thank you for the review. I checked several mcif entries from MAGDATA and they all record _parent_space_group.child_transform_pp_abc, _parent_space_group.transform_pp_abc values as strings.

I have opened a separate issue (https://github.com/COMCIFS/magnetic_dic/issues/66) which deals with the addition of the _pp items and will merge this PR with the corrections to avoid blocking the release.