Closed nils-wittemeier closed 4 months ago
@zerothi could you have a look at this and see if it makes sense to you?
One thing that sill 'bothers' me, is that I am not 100% certain of the units of the eigenmodes stored by Vibra. I think it should be in Bohr.
Once that is clarified and the units correctly converted to Angstrom in here.
One thing that sill 'bothers' me, is that I am not 100% certain of the units of the eigenmodes stored by Vibra. I think it should be in Bohr.
Once that is clarified and the units correctly converted to Angstrom in here.
I believe vibra uses default Siesta units, so it will be in Ry^2/Bohr^2, however the frequencies are in cm*-1. And everything is written as-is*. The eigenmodes have unit-length, so I think they should be unit-less, no? They are basically the zheev
values times 1/sqrt(mass_ia)
.
Nevermind, eigenmodes should be normalized. So no units ...
If you have no further comments, I think it should be ready.
Now all we need are tests ;)
And if you could install pre-commit
it would make it check the code, see here https://github.com/zerothi/sisl/blob/main/CONTRIBUTING.md#we-develop-with-github
for how to use it.
According to https://github.com/zerothi/sisl-files/issues/14 feel free to add all files, I'll move them over to a new repo.
One thing that sill 'bothers' me, is that I am not 100% certain of the units of the eigenmodes stored by Vibra. I think it should be in Bohr. Once that is clarified and the units correctly converted to Angstrom in here.
I believe vibra uses default Siesta units, so it will be in Ry^2/Bohr^2, however the frequencies are in cm**-1. And everything is written as-is. The eigenmodes have unit-length, so I think they should be unit-less, no? They are basically the
zheev
values times1/sqrt(mass_ia)
.
Actually, this is a discussion point, should phonon modes always have the factor 1/sqrt(mass)
or should they just be normalized? What is your opinion here? Because we need to streamline this. :)
One thing that sill 'bothers' me, is that I am not 100% certain of the units of the eigenmodes stored by Vibra. I think it should be in Bohr. Once that is clarified and the units correctly converted to Angstrom in here.
I believe vibra uses default Siesta units, so it will be in Ry^2/Bohr^2, however the frequencies are in cm**-1. And everything is written as-is. The eigenmodes have unit-length, so I think they should be unit-less, no? They are basically the
zheev
values times1/sqrt(mass_ia)
.Actually, this is a discussion point, should phonon modes always have the factor
1/sqrt(mass)
or should they just be normalized? What is your opinion here? Because we need to streamline this. :)
In most cases, I would think the scaled version with energies is needed. But, maybe it would make sense to implement this in a way that the EigenmodePhonon can return both? How could we handle those cases where the atomic masses are unknown? Maybe passing a parent object with masses has to be strictly mandatory?
One thing that sill 'bothers' me, is that I am not 100% certain of the units of the eigenmodes stored by Vibra. I think it should be in Bohr. Once that is clarified and the units correctly converted to Angstrom in here.
I believe vibra uses default Siesta units, so it will be in Ry^2/Bohr^2, however the frequencies are in cm**-1. And everything is written as-is. The eigenmodes have unit-length, so I think they should be unit-less, no? They are basically the
zheev
values times1/sqrt(mass_ia)
.Actually, this is a discussion point, should phonon modes always have the factor
1/sqrt(mass)
or should they just be normalized? What is your opinion here? Because we need to streamline this. :)In most cases, I would think the scaled version with energies is needed. But, maybe it would make sense to implement this in a way that the EigenmodePhonon can return both? How could we handle those cases where the atomic masses are unknown? Maybe passing a parent object with masses has to be strictly mandatory?
I agree, that would mean that DynamicalMatrix
should be fixed to return mass-scaled eigenvectors. It would also make sense since the mass-matrix can be thought of as the overlap matrix. So lets do that, we need to change DynamicalMatrix
then (lets do that in a separate PR).
Agreed, that when the masses are unknown, it should fail in some way.
Energies should just be in eV as you are doing it.
One thing that sill 'bothers' me, is that I am not 100% certain of the units of the eigenmodes stored by Vibra. I think it should be in Bohr. Once that is clarified and the units correctly converted to Angstrom in here.
I believe vibra uses default Siesta units, so it will be in Ry^2/Bohr^2, however the frequencies are in cm**-1. And everything is written as-is. The eigenmodes have unit-length, so I think they should be unit-less, no? They are basically the
zheev
values times1/sqrt(mass_ia)
.Actually, this is a discussion point, should phonon modes always have the factor
1/sqrt(mass)
or should they just be normalized? What is your opinion here? Because we need to streamline this. :)In most cases, I would think the scaled version with energies is needed. But, maybe it would make sense to implement this in a way that the EigenmodePhonon can return both? How could we handle those cases where the atomic masses are unknown? Maybe passing a parent object with masses has to be strictly mandatory?
I agree, that would mean that
DynamicalMatrix
should be fixed to return mass-scaled eigenvectors. It would also make sense since the mass-matrix can be thought of as the overlap matrix. So lets do that, we need to changeDynamicalMatrix
then (lets do that in a separate PR).Agreed, that when the masses are unknown, it should fail in some way.
Energies should just be in eV as you are doing it.
Acually, I have been to quick. Already now, all DynamicalMatrices created from sisl
are mass-scaled. That is essentially the definition of the DynamicalMatrix
without the mass-scaling, it will be the Force constants (Hessian). So we shouldn't do anything :)
Related to the discussion on the phase of the eigenvectors in vibra. Do we need to change the gauge when reading, to be consistent with the phonon modes obtained from the DynamicalMatrix
object in sisl?
Related to the discussion on the phase of the eigenvectors in vibra. Do we need to change the gauge when reading, to be consistent with the phonon modes obtained from the
DynamicalMatrix
object in sisl?
Not really (I think), the gauge
variable is passed together with the object, so a simple change_gauge
would be fine in any script, it simply is a noop if the gauge is correct.
Related to the discussion on the phase of the eigenvectors in vibra. Do we need to change the gauge when reading, to be consistent with the phonon modes obtained from the
DynamicalMatrix
object in sisl?Not really (I think), the
gauge
variable is passed together with the object, so a simplechange_gauge
would be fine in any script, it simply is a noop if the gauge is correct.
Oh, you are correct, the gauge should be added to the info
parameter! Yes, yes, yes! ;-)
Attention: Patch coverage is 89.20863%
with 15 lines
in your changes are missing coverage. Please review.
Project coverage is 87.28%. Comparing base (
9c0317d
) to head (6c94a27
). Report is 1 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
src/sisl/io/siesta/vibra.py | 86.84% | 15 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Related to the discussion on the phase of the eigenvectors in vibra. Do we need to change the gauge when reading, to be consistent with the phonon modes obtained from the
DynamicalMatrix
object in sisl?
Related to the phase of the eigenvectors, what is your view on the phases in sisl
?
I have had problems comparing vibra and sisl
, but I think mainly because of the Wigner-Seitz reduction, which I don't have in sisl (it should be there but I don't have the competences and time to delve in to this).
What is missing here? Should I do something?
Sorry for going MIA.
As far as I can tell, three things are missing:
I'll update the PR tonight with missing bit.
Gauge was already mentioned in the info dict.
Units conversion is changed now, and I added a very basic test.
Relevant files are added here
I think once the example is in, we can update the submodule, and this should be done
Example is in!
Should be ready now
Found out the order of the states were wrong, they should be [istate, iatom, range(3)]
, I will change this with some more changes, I'll ping you!
@nils-wittemeier could you have a look at 1ad80f31c?
Of course
See title.