Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8f7f01c) 83.80% compared to head (6262da1) 83.80%.

@@           Coverage Diff            @@
##           develop     #378   +/-   ##
========================================
  Coverage    83.80%   83.80%           
========================================
  Files           24       24           
  Lines         8167     8167           
========================================
  Hits          6844     6844           
  Misses        1323     1323           

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

I am confused. It seems those are specified for space group dataset in the source code. Why were those for layer group blank?

The axis choice for layer groups 3, 4 and 6 were blank because they only have one choice, i.e. the axis of the symmetry operation along $\mathbf{c}$. So, when I write the Hall symbol table for layer groups, I dropped the choice following the behavior of ITB.

It is my fault. I ignored the caption under the Table A1.4.2.7 Hall symbols of ITB. It says

Where no code is given the first choice listed below applies

Then it listed the default choice for different crystal systems. We did not stipulate the default unique axis for layer groups as it may be $\mathbf{a}$ or $\mathbf{c}$ for different groups (or maybe we imply it to be $\mathbf{c}$?)

Some of the choice specified for space group dataset in the source code are also blank. The most obviously one is P 1, I think it is following ITB, i.e., blank denotes the default choice. Should we fill all of them? I do not think it is a good idea, as something like 'origin choice 1' is also confusing for people not familiar with crystallography.

@site-g, thanks for your explanation.

why, for layer group 3: p 1 1 2, 4: p 1 1 m and 6:p 1 1 2/m, were those blank?

@site-g, is my understanding correct?

• SpacegroupType layer_group_types were created based on SpacegroupType spacegroup_types. Or is there any reference?
• For example of space group No.3, there are three choices for space group, but only one for layer group as shown below. So you didn't specify the choice for the layer group. But now you realized that we have to be specify it to make set_layer_monocli work.

layer group

    {3, "C2^1  ", "p 2             ", "p 1 1 2                        ",
     "p 1 1 2            ", "p112      ", "     ", PRIMITIVE, 3}, /*   3 */

space group

    {3, "C2^1  ", "P 2y            ", "P 2 = P 1 2 1                  ",
     "P 1 2 1            ", "P2        ", "b    ", PRIMITIVE, 3}, /*   3 */
    {3, "C2^1  ", "P 2             ", "P 2 = P 1 1 2                  ",
     "P 1 1 2            ", "P2        ", "c    ", PRIMITIVE, 3}, /*   4 */
    {3, "C2^1  ", "P 2x            ", "P 2 = P 2 1 1                  ",
     "P 2 1 1            ", "P2        ", "a    ", PRIMITIVE, 3}, /*   5 */

SpacegroupType layer_group_types were created based on SpacegroupType spacegroup_types. Or is there any reference?

The reference is Table 1.2.6.1 and Chapter 4 of ITE.

For example of space group No.3, there are three choices for space group, but only one for layer group as shown below. So you didn't specify the choice for the layer group.

For example of layer group No.3 p 1 1 2 , Chapter 4 of ITE only shows one choice for this layer group. So I didn't specify the choice for the layer group. While for layer group No.8 p 2 1 1, Table 1.2.6.1 shows two different choices, so I specified them in SpacegroupType layer_group_types. The three different choices of these two layer groups correspond to the three different choices of space group No.3.

But now you realized that we have to be specify it to make set_layer_monocli work.

That's true.

@site-g, thanks. It became clear.

@lan496, I approved.

@site-g This force push (https://github.com/spglib/spglib/compare/1f9aa8c002414be3d3037546da88d44e16eef8b7..6262da18f69492194d685fd2d4ab21f8d2fd91f3) looks another PR.
Can you revert it from this PR?

Sorry, that force push seems to be blocked by branch protection rule. Now, I have merged this PR.