Move to spglib_jll #265
Move to spglib_jll #265
Conversation
There was a problem hiding this comment.
@louisponet Thanks very much for going through the effort to do this! I have a few nits, mostly wrt. formatting and naming. Regarding the implementation, what mainly bothers me is that some of your functions bleed out the C types from spglib, which should really be internal and only live within spglib.jl from my point of view.
I hope I was not too picky 😄.
test/bzmesh.jl
Outdated
| is_shift = ifelse.(kshift .== 0, 0, 1) | ||
| _, grid = spglib.get_stabilized_reciprocal_mesh(kgrid_size, identity, | ||
| num_kpts, _, grid = DFTK.spglib_get_stabilized_reciprocal_mesh(kgrid_size, identity, |
There was a problem hiding this comment.
If you don't use num_kpts please also use an underscore for that arg.
There was a problem hiding this comment.
Actually I just had an idea about this: Basically what this tries to test is that we agree with spglib, so from that point of view the best way to test would be (contrary to what I said before) to directly do a ccall into spglib_jll from the test and bypass as much of the extra icing DFTK spglib.jl adds on top. You're call if you think that's too much effort to do or not ...
There was a problem hiding this comment.
Also the line starts to become a bit long ;)
test/bzmesh.jl
Outdated
| is_shift = ifelse.(kshift .== 0, 0, 1) | ||
| _, grid = spglib.get_stabilized_reciprocal_mesh(kgrid_size, identity, | ||
| num_kpts, _, grid = DFTK.spglib_get_stabilized_reciprocal_mesh(kgrid_size, identity, |
There was a problem hiding this comment.
Actually I just had an idea about this: Basically what this tries to test is that we agree with spglib, so from that point of view the best way to test would be (contrary to what I said before) to directly do a ccall into spglib_jll from the test and bypass as much of the extra icing DFTK spglib.jl adds on top. You're call if you think that's too much effort to do or not ...
test/bzmesh.jl
Outdated
| is_shift = ifelse.(kshift .== 0, 0, 1) | ||
| _, grid = spglib.get_stabilized_reciprocal_mesh(kgrid_size, identity, | ||
| num_kpts, _, grid = DFTK.spglib_get_stabilized_reciprocal_mesh(kgrid_size, identity, |
There was a problem hiding this comment.
Also the line starts to become a bit long ;)
|
Ah also could you rebase against master at some point to prepare for the merge @louisponet, thanks! |
|
In retrospect I'm not sure our tilde and not tilde notation was very good. We should maybe have chosen Sstar for what we call S and S for what we call Stilde, to emphasize Sstar works in reciprocal space and S in real space? Or is there any standard conventions here? |
|
Well the other point is that there is |
|
The parallelism between S and Stilde and tau and tautilde is not very good, actually. Stilde is a rotation in real space, S a rotation in recp space. tautilde is a translation in real space. tau is also a translation in real space |
|
I'll open another issue for that |
|
Some nitpicking but apart from that good for me! |
All tests work. Not sure if there are the same amount of warnings as on the master branch tests.