Draft: add vectorsSileSIESTA for parsing vibra output

[nils-wittemeier]

See title.

[nils-wittemeier]

@zerothi could you have a look at this and see if it makes sense to you?

[zerothi]

Looks fine! Some comments here and there ;)

[nils-wittemeier]

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.

[zerothi]

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).

[nils-wittemeier]

Nevermind, eigenmodes should be normalized. So no units ...

If you have no further comments, I think it should be ready.

[zerothi]

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 zerothi/sisl-files#14 feel free to add all files, I'll move them over to a new repo.

[zerothi]

minor comments

[zerothi]

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).

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. :)

[nils-wittemeier]

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).

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?

[zerothi]

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).

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.

[zerothi]

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).

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.

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 :)

[nils-wittemeier]

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?

[zerothi]

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.

[zerothi]

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.

Oh, you are correct, the gauge should be added to the info parameter! Yes, yes, yes! ;-)

[codecov]

Codecov Report

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 ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #742      +/-   ##
==========================================
- Coverage   87.30%   87.28%   -0.03%     
==========================================
  Files         394      396       +2     
  Lines       50409    50445      +36     
==========================================
+ Hits        44011    44032      +21     
- Misses       6398     6413      +15     

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

[zerothi]

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).

[zerothi]

What is missing here? Should I do something?

[nils-wittemeier]

Sorry for going MIA.

As far as I can tell, three things are missing:

• providing the correct gauge in the info dict
• adding a test
• using your suggestion for the unit conversion

I'll update the PR tonight with missing bit.

[nils-wittemeier]

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

[nils-wittemeier]

I think once the example is in, we can update the submodule, and this should be done

[zerothi]

Example is in!

[nils-wittemeier]

Should be ready now

[zerothi]

I'll merge and remove the sc argument

[zerothi]

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!

[zerothi]

@nils-wittemeier could you have a look at 1ad80f3?

[nils-wittemeier]

Of course