Fixes to BrillouinSpglibExt as suggested in #26

[mfherbst]

No description provided.

[codecov-commenter]

Codecov Report

Merging #27 (7832df1) into master (1aa1e32) will decrease coverage by 0.30%.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
- Coverage   85.95%   85.65%   -0.30%     
==========================================
  Files          14       14              
  Lines         968      969       +1     
==========================================
- Hits          832      830       -2     
- Misses        136      139       +3     

Files	Coverage Δ
ext/BrillouinSpglibExt.jl	100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[thchr]

Thanks! This should probably also bump the version of Spglib in Project.toml:

Brillouin.jl/Project.toml

Line 35 in 1aa1e32

Spglib = "0.6"

[mfherbst]

For sure ! Thanks @thchr

[mfherbst]

These failures look like they are due to changes in the ordering of matrices (rows versus columns etc.) @singularitti any idea which of your changes could cause this ?

[singularitti]

I probably transposed some matrices to align with Spglib's mathematical convention. I will have a look at these errors.

[mfherbst]

Got it. It actually was that @singularitti plus a small inconsistency in the code.

[singularitti]

Alright, I've reviewed your code and compared it to my convention. It appears that Michael's fix is indeed correct.

In Spglib's convention, when you transform the reference axes, you apply the transformation matrix by multiplying it on the right, denoted as P:

However, when you rotate the axes during idealization to get dataset.std_lattice, you multiply the rotation matrix R on the left:

I realized that in versions prior to v0.8, I was using the inconsistent convention, which resulted in the rotation matrix being the transpose of the current convention. Interestingly, this happened to align with your convention.

I'm also checking if dataset.transformation_matrix might be affected by this change. @mfherbst, did you use dataset.transformation_matrix in your code?

Apart from that, I have conducted extensive tests since v0.8, and it seems that the current convention is indeed the correct one.

I have a couple of suggestions:

1. Consider changing points(kp)[lab] = rotation' * kv to points(kp)[lab] = transpose(rotation) * kv. Using transpose makes it more explicit and easier to understand.

2. Instead of eachcol, you can use pRs = SVector{3}(Spglib.basisvectors(dset.std_lattice)) and pRs_original = SVector{3}(Spglib.basisvectors(cell.lattice). These are part of the official API I provided, which can enhance code readability and maintainability.

Feel free to let me know your thoughts or if you have any further questions!

[thchr]

This seems to work now, at least on the latest Julia version. I don't quite understand the errors on Julia master, but I suppose that could well be a master-issue rather than an issue related to this PR.

Let me know when you are happy with the PR @mfherbst and I'll merge.

[thchr]

And thanks for double checking everything @singularitti: really nice to have it settled and carefully checked.

[mfherbst]

Thanks for the comments @singularitti. I changed the code according to your second suggestion. I don't think the first is worth the additional characters 😄: In Julia the tick is quite wide-spread to transpose a matrix and the comment should make clear why.

[mfherbst]

Good to go from my end now @thchr !

[thchr]

Brilliant; many thanks!