irreducible-representations / irrep

GNU General Public License v3.0
62 stars 31 forks source link

Adding hdf5 support #39

Closed MSteggles closed 3 months ago

MSteggles commented 2 years ago

Hi,

Here's the modification for hdf5 support, little bit qnd but I think that's all it needs to be. Sets a flag for hdf5 if the wavefunctions are found in that format and reads them to the same variables as in the dat case. I've tested on .dat and .hdf5 with and without SOC, I'm pretty sure it's working. I personally also had to change the import of 'isround' in bandstructure.py to irrep.utility from irrep.__aux but I don't understand why.

Cheers, Matthew

stepan-tsirkin commented 2 years ago

@MSteggles , thank you for your contribution. As ypu can see, the automaed testys have failed with message Python version >= 3.8 required.

I disabnled checks on the old python 3.7, and enabled for python 3.8 and 3.9. Please, update your branch from the master branch, and we'll see if the checks work.

Further, we will need a small test to make sure the HDF5 support will not be broken in future. For that a minimal (not obligatore physically meaningful) example is needed.

MSteggles commented 2 years ago

Oh dear, I'll fix this as soon as I have some free time. Thank you

MSteggles commented 2 years ago

Hi;

Apologies for the delay, I've been extremely busy with PhD work (handing in ~Christmas), thanks so much for your patience! Hm yeah, just had a look - I'm a bit confused as h5py works from python

=3.7, so I really don't know where the problem has come in. I'll have a look and see if I can fix it. I'm also a little unsure of what you want:

1) Are you saying I just need to disable checks to see if the 3.8, 3.9 builds complete fine? 2) What would you like from a test? All it does is add the ability to read that file format - would reading a given file and checking it's correct against the .dat version suffice? The use of hdf5 comes from deep within QEspresso

Looking forward to hearing from you

Best, Matthew

On Sun, Apr 17, 2022 at 3:27 PM Stepan Tsirkin @.***> wrote:

@MSteggles https://github.com/MSteggles , thank you for your contribution. As ypu can see, the automaed testys have failed with message Python version >= 3.8 required.

I disabnled checks on the old python 3.7, and enabled for python 3.8 and 3.9. Please, update your branch from the master branch, and we'll see if the checks work.

Further, we will need a small test to make sure the HDF5 support will not be broken in future. For that a minimal (not obligatore physically meaningful) example is needed.

— Reply to this email directly, view it on GitHub https://github.com/stepan-tsirkin/irrep/pull/39#issuecomment-1100890014, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHO7MYYMUWZCVWAJKKZ7AWDVFQNWNANCNFSM5SNGG3FQ . You are receiving this because you were mentioned.Message ID: @.***>

stepan-tsirkin commented 2 years ago

@MSteggles , no worries, we all contribute when we can

basically, the check on python 3.7 should be disabled in the file .github/workflows)/tests.yml , and it already is on the master branch:

https://github.com/stepan-tsirkin/irrep/blob/670328922afe4d78ef390cf83f1bc314ce1f5893/.github/workflows/tests.yml#L12

However, in your branch it is only 3.7, which is outdated:

https://github.com/MSteggles/irrep/blob/d74d4ebaaefd5bf14afdf607c86d76ca95d48c58/.github/workflows/tests.yml#L12

So, first of all merge my master branch into your branch, that should take incorporate my latest commits, including this

In principle, a test should read the data, run the calculation and make sure that the output is correct (i.e. compare with some output that you are sure to be correct). I just realized that we do not have any tests for QE, but have for other codes.

So, to create a test yopu need :

  1. in the examples folder create another folder with an example, containing all input data (output of DFT code required to run irrep). The size of this data should be kept at a minimum (not even required to be physically meaningful), so minimal system (1atom per unit cell), minimal cutoff, minimal number of k-points and bands etc)
  2. In https://github.com/MSteggles/irrep/blob/master/irrep/tests/test_cli.py create another function with name starting from test_ that will check the output for matching some key correct strings. All by analogy

You may create two folders, in one only hdf5 data, in the other only the .dat files in the .save directory. So, there will be two tests (but the output may be done the same)

If you struggle with that, do not hesitate to ask me again.

MSteggles commented 2 years ago

Excellent stuff :-) Thanks for the help, I'll get on those and get in touch if I have trouble. Cheers, Matthew

On Mon, May 2, 2022 at 7:49 PM Stepan Tsirkin @.***> wrote:

@MSteggles https://github.com/MSteggles , no worries, we all contribute when we can

basically, the check on python 3.7 should be disabled in the file .github/workflows)/tests.yml , and it already is on the master branch:

https://github.com/stepan-tsirkin/irrep/blob/670328922afe4d78ef390cf83f1bc314ce1f5893/.github/workflows/tests.yml#L12

However, in your branch it is only 3.7, which is outdated:

https://github.com/MSteggles/irrep/blob/d74d4ebaaefd5bf14afdf607c86d76ca95d48c58/.github/workflows/tests.yml#L12

So, first of all merge my master branch into your branch, that should take incorporate my latest commits, including this https://github.com/stepan-tsirkin/irrep/commit/5f04724eba2a61c69038ef7e8656d7c7c18d3aea

In principle, a test should read the data, run the calculation and make sure that the output is correct (i.e. compare with some output that you are sure to be correct). I just realized that we do not have any tests for QE, but have for other codes.

So, to create a test yopu need :

  1. in the examples folder create another folder with an example, containing all input data (output of DFT code required to run irrep). The size of this data should be kept at a minimum (not even required to be physically meaningful), so minimal system (1atom per unit cell), minimal cutoff, minimal number of k-points and bands etc)
  2. In https://github.com/MSteggles/irrep/blob/master/irrep/tests/test_cli.py create another function with name starting from test_ that will check the output for matching some key correct strings. All by analogy

You may create two folders, in one only hdf5 data, in the other only the .dat files in the .save directory. So, there will be two tests (but the output may be done the same)

If you struggle with that, do not hesitate to ask me again.

— Reply to this email directly, view it on GitHub https://github.com/stepan-tsirkin/irrep/pull/39#issuecomment-1115241905, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHO7MYYVGNFL6U255M333D3VIAPTDANCNFSM5SNGG3FQ . You are receiving this because you were mentioned.Message ID: @.***>

stepan-tsirkin commented 3 months ago

implemented in #85

MSteggles commented 3 months ago

Awesome! I (obviously) forgot about this a long time ago. Happy it's resolved and I hope it wasn't overly inconvenient.

M