Adding band structure parser for cp2k version 8.1.

[hdsassnick]

Hello,

I would firstly like to thank you for the work and effort developing the AiiDA-plugin for cp2k.
It is a great addition to the AiiDA-library and facilitates my work a lot.

I noticed that for the new version 8.1 of cp2k the output format of the band structure has changed and is incompatible with previous versions.
I would like to contribute an adaption of the advanced-output parser which should work with the most recent versions of cp2k
but still keeps the downwards compatibility to older versions of the program.

I would be very happy if the snippet of additional code can be helpful.
This is my first pull request. Apologies if I hadn't done it completely correctly.

Best wishes,
Holger

[yakutovicha]

Hey @hdsassnick, thanks a lot.

I would be very happy if the snippet of additional code can be helpful.
This is my first pull request. Apologies if I hadn't done it completely correctly.

No worries, I think you did a great job.

Before I start reviewing the PR, can you please fix the pre-commit issues? It basically just adds two empty lines, see here

[hdsassnick]

Hi, should work now.

[hdsassnick]

Hi,

I was just wondering if there was any news concerning the review of the pull request.

[yakutovicha]

Hey @hdsassnick, sorry for the long wait.

I had a closer look at your implementation and I have a couple of suggestions. Generally, I think it would be a good idea to reuse the code that is there already. For instance, the parts of the code that don't work with the 8.1 version of CP2K are:

aiida-cp2k/aiida_cp2k/utils/parser.py

Line 301 in 60d5183

pattern = re.compile(".*?Nr.*?Spin.*?K-Point.*?", re.DOTALL)

and

aiida-cp2k/aiida_cp2k/utils/parser.py

Lines 312 to 317 in 60d5183

	spin = int(splitted[3])
	kpoint = tuple(float(p) for p in splitted[-3:])
	kpoint_n_lines = int(math.ceil(int(selected_lines[current_line + 1]) / 4))
	band = [
	float(v) for v in " ".join(selected_lines[current_line + 2:current_line + 2 + kpoint_n_lines]).split()
	]

Herein, I think it would be better to modify the _parse_bands function as follows

def _parse_band_before81(band_lines):
    put the old code here

def _parse_band_after81(band_lines):
    put the new code here

def _parse_bands(lines, n_start, cp2k_version):
    if cp2k_version < 8.1:
        pattern = re.compile(".*?Nr.*?Spin.*?K-Point.*?", re.DOTALL)
        parse_band_function = _parse_band_before81
    else:
        pattern = re.compile(".*?Point.*?Spin.*?", re.DOTALL) # didn't try it myself
        parse_band_function = _parse_band_after81

I think this approach allows us to write shorter and clearer code.

Let me know if you agree.

[hdsassnick]

Hi @yakutovicha ,
Thank you very much for the suggestions. Indeed I forgot to remove some print statements that I just included for testing the function, thank you for pointing that out.
I think the differences between the two functions is actually a bit larger, e.g. the number of bands is not printed at the head of each k-point in the new cp2k version.
Thus, the current function for cp2k v8.1 is checking line-by-line if an eigenvalue needs to be added to the list. In contrast to that, in cp2k v7.1 the number of eigenvalues is known beforehand, and this information is then being used to add all eigenvalues in one go.
That is why I have a few doubts that merging the two versions of "_parse_bands" would simplify the code by a large extent. Nevertheless, I will give it a try.

[hdsassnick]

Hi @yakutovicha ,

I tried to implement your suggestions. I'm not sure if I did it the way you intended.
Could you please have another look at the implementation?

[yakutovicha]

Hi @yakutovicha ,

I tried to implement your suggestions. I'm not sure if I did it the way you intended.
Could you please have another look at the implementation?

@hdsassnick I decided to continue the development a little bit on my own - sorry for that. I just thought that it would be easier for me to implement what I had in mind directly rather than explain what I actually meant.

... e.g. the number of bands is not printed at the head of each k-point in the new cp2k version.

I think that is a pretty minor difference that could be resolved by employing try...except - please have a look at the code.

That is why I have a few doubts that merging the two versions of "_parse_bands" would simplify the code to a large extent.

Let me know what you think of the current implementation.

[hdsassnick]

Hi @yakutovicha ,
Thanks so much! This new version looks fantastic.
I just would have a small addition concerning the case when labels are not given (I also overlooked this detail in my first commit, sorry).

[yakutovicha]

alright, I think this is ready to go.

Thanks, @hdsassnick for your contribution and for using the plugin. You are very welcome to create issues if something isn't working correctly or issues/PRs in case you want to suggest further improvements!

[yakutovicha]

This will be available in the next minor release of the plugin.