Fix order of primitive basis for get_magnetic_symmetry

[lan496]

Fixes: #370

[lan496]

@atztogo This PR fixes my mistake of not transposing in Python API. I've checked other lattices like std_lattice are correctly handled.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9c1099c) 83.80% compared to head (6bbcb7d) 83.80%.
Report is 6 commits behind head on develop.

Additional details and impacted files

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

Flag	Coverage Δ
c_api	77.18% <ø> (ø)
fortran_api	56.19% <ø> (ø)
python_api	80.47% <ø> (ø)
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.

[LecrisUT]

I think this needs a bit more rethinking. Why are the lattices transposed when both C and python use the same row-major?

Also, isn't all of the api supposed to go through spg.dataset?

[atztogo]

I think this needs a bit more rethinking. Why are the lattices transposed when both C and python use the same row-major?

They are transposed each other. https://spglib.readthedocs.io/en/latest/python-spglib.html#crystal-structure-cell

This is due to historical reasons by my decisions.
There was no python interface initially. I prefer to have basis vectors in a 3x3 C-array resembling $(\mathbf{a} \ \mathbf{b} \ \mathbf{c})$ lattice, i.e., a, b, c are put in the array like column vectors although I knew those in the row vectors are popular in condensed matter scientific codes.

I started to develop phonopy in python and so I wanted to make the python interface of spglib. I thought it was a good idea to follow the convention of the ASE's Atoms class, because at that time, python was not popular in science, but ASE and GPAW did good jobs in implementation in python. Of course I could never expecte that spglib becomes so popular, so I decided it without deep consideration.

Also, isn't all of the api supposed to go through spg.dataset?

It is done in C for space group and magnetic space group. But the transpose has to be made for basis vecotrs in python. We made this mistake due to complication introduced to have compatibility to my old implementation.

If I understand correctly, get_magnetic_symmetry is a successor of my old implementation existed in v1, and get_magnetic_symmetry_dataset is really new at v2. My original implementation had no ability to identify magnetic space group types and was not crystallographically well defined in the treatment of spins. @lan496 not only fixed the confusion of treatment of spins in my original implementation but also developed the identification of the magnetic space group types. Without comparing a set of symmetry operations with the magnetic space group type, it is not easy to obtain a set of symmetry operations that forms a group, i.e., my original implementation lacked this ability. Then @lan496 updated get_magnetic_symmetry to use the magnetic dataset as the back end to fix this issue.

[singularitti]

I also find the convention confusing, particularly the Python interface's need for lattice transposition. What's worse is that Julia uses column-major order, in contrast to C and Python's row-major order. This difference added an extra layer of confusion during the development of the Julia interface. To address this, I wrote documentation for my users. Nevertheless, I'm glad it's all sorted out.

@lan496 not only fixed the confusion of treatment of spins in my original implementation but also developed the identification of the magnetic space group types. Without comparing a set of symmetry operations with the magnetic space group type, it is not easy to obtain a set of symmetry operations that forms a group, i.e., my original implementation lacked this ability.

Wow, that's some awesome work @lan496!

[atztogo]

If we have many wrappers, it may be good to have consensus about the definition of the basis vector matrix, where we can consider the python wrapper is an exception (because it is too late) and the others may, for example, follow the memory order in C. By this, in Fortran, it is transposed. However, in fact, we can introduce another confusion for atomic points either (num_points, 3) or (3, num_points). Finally @singularitti is right, documentation should ease the issue and developers have to be careful about it, if people think it is a better solution than developing an spglib-althernative with your favorite array definitions from scratch.

[lan496]

You may (already) realize column-major basis vectors are so natural in implementing crystallographic algorithms, but at the same time, we want to use row-major ones in the row-major preferred languages like Python 😇
For the Python interface, this PR will eliminate the issues about the order of basis vectors although it will not be a holistic solution.

@atztogo Can I merge this PR?

[LecrisUT]

For the Python/C interface, the column/row major is not a problem, and the choice of column vectors is also perfectly fine. The issues here are rather:

• Inconsistency between Python/C interface if one version does the transpose and another doesn't. Regardless of the data-structure underneath, the mathematical notation lattice[row][column] is the same for all languages. This notation we must make sure is consistent
• Making sure the ownership of the objects is consistent. In most cases, the wrapper should copy the content and make a native array of it. Using the transpose, would guarantee the data is copied

[atztogo]

@atztogo Can I merge this PR?

Have you seen my comment above? Transpose of numpy array doesn't change data in memory, i.e., the order becomes 'F'. I prefer order='C' if the overhead to make it so is small.

[atztogo]

In [1]: import numpy as np

In [2]: a = np.arange(9).reshape(3,3)

In [3]: a
Out[3]:
array([[0, 1, 2],
       [3, 4, 5],
       [6, 7, 8]])

In [4]: a.flags
Out[4]:
  C_CONTIGUOUS : True
  F_CONTIGUOUS : False
  OWNDATA : False
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False

In [5]: np.transpose(a).flags
Out[5]:
  C_CONTIGUOUS : False
  F_CONTIGUOUS : True
  OWNDATA : False
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False

In [6]: np.array(a.T).flags
Out[6]:
  C_CONTIGUOUS : False
  F_CONTIGUOUS : True
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False

In [7]: np.array(a.T, order='C').flags
Out[7]:
  C_CONTIGUOUS : True
  F_CONTIGUOUS : False
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  WRITEBACKIFCOPY : False

[lan496]

@atztogo Do you add comments on the code? If so, I cannot see it now. Anyway, I copied the array.

[atztogo]

Sorry, I didn't realize that I had to push the button to submit request the change.

[lan496]

Not a problem at all. I have also had the same trouble before.