Skip to content

Fix arithmetic crystal class for -6m2P and -62mP #367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 4, 2023
Merged

Fix arithmetic crystal class for -6m2P and -62mP #367

merged 1 commit into from
Dec 4, 2023

Conversation

lan496
Copy link
Member

@lan496 lan496 commented Nov 15, 2023

Arithmetic crystal classes for P-6m2 (No. 187), P-6c2 (No. 188), P-62m (No. 199), and P-62c (No. 190) seem to be opposite.

char arithmetic_crystal_class_symbols[74][7] in arithmetic.c looks to follow the order in IUCr's dictionary.
Then, arithmetic number 56 is -62mP, and 57 is -6m2P.
Thus, the arithmetic crystal class for P-6m2 (No. 187) is -6m2P (57) not -62mP (56).

After this PR, these arithmetic crystal classes for 187-190 are fixed as follows:

for hall_number in [481, 482, 483, 484]:
   ...:     data = get_spacegroup_type(hall_number)
   ...:     print(data)

{'number': 187, 'international_short': 'P-6m2', 'international_full': 'P -6 m 2', 'international': 'P -6 m 2', 'schoenflies': 'D3h^1', 'hall_number': 481, 'hall_symbol': 'P -6 2', 'choice': '', 'pointgroup_international': '-6m2', 'pointgroup_schoenflies': 'D3h', 'arithmetic_crystal_class_number': 57, 'arithmetic_crystal_class_symbol': '-6m2P'}
{'number': 188, 'international_short': 'P-6c2', 'international_full': 'P -6 c 2', 'international': 'P -6 c 2', 'schoenflies': 'D3h^2', 'hall_number': 482, 'hall_symbol': 'P -6c 2', 'choice': '', 'pointgroup_international': '-6m2', 'pointgroup_schoenflies': 'D3h', 'arithmetic_crystal_class_number': 57, 'arithmetic_crystal_class_symbol': '-6m2P'}
{'number': 189, 'international_short': 'P-62m', 'international_full': 'P -6 2 m', 'international': 'P -6 2 m', 'schoenflies': 'D3h^3', 'hall_number': 483, 'hall_symbol': 'P -6 -2', 'choice': '', 'pointgroup_international': '-6m2', 'pointgroup_schoenflies': 'D3h', 'arithmetic_crystal_class_number': 56, 'arithmetic_crystal_class_symbol': '-62mP'}
{'number': 190, 'international_short': 'P-62c', 'international_full': 'P -6 2 c', 'international': 'P -6 2 c', 'schoenflies': 'D3h^4', 'hall_number': 484, 'hall_symbol': 'P -6c -2c', 'choice': '', 'pointgroup_international': '-6m2', 'pointgroup_schoenflies': 'D3h', 'arithmetic_crystal_class_number': 56, 'arithmetic_crystal_class_symbol': '-62mP'}

Actually, I think the order of arithmetic crystal classes for -62mP and -6m2P is strange: other cases like -42mP and -4m2P are ordered the same as space group types.

@lan496 lan496 requested a review from atztogo November 15, 2023 11:07
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8fab0b0) 83.68% compared to head (1fbdacc) 83.68%.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop     #367   +/-   ##
========================================
  Coverage    83.68%   83.68%           
========================================
  Files           24       24           
  Lines         8144     8144           
========================================
  Hits          6815     6815           
  Misses        1329     1329
Flag Coverage Δ
c_api 76.45% <ø> (ø)
fortran_api 56.29% <ø> (ø)
python_api 80.54% <ø> (ø)
unit_tests 1.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lan496
Copy link
Member Author

lan496 commented Dec 2, 2023

@atztogo Can you review this PR when you have time?

atztogo
atztogo approved these changes Dec 3, 2023
View reviewed changes
Copy link
Collaborator

@atztogo atztogo left a comment

Choose a reason for hiding this comment

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

I forgotten to approve this. Please always call me in this way any number of times.

@lan496 lan496 enabled auto-merge December 3, 2023 02:30
@lan496 lan496 force-pushed the fix-arithmetic-62mP branch from 1fbdacc to 9392fe5 Compare December 4, 2023 05:55
@lan496 lan496 merged commit 163f143 into spglib:develop Dec 4, 2023
@lan496 lan496 deleted the fix-arithmetic-62mP branch December 4, 2023 10:32
@Porkepix Porkepix mentioned this pull request Dec 8, 2023
Merged
@LecrisUT LecrisUT added this to the 2.2 milestone Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atztogo atztogo atztogo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.2
Development

Successfully merging this pull request may close these issues.
4 participants
@lan496 @codecov-commenter @atztogo @LecrisUT