Improve Bandoverlaps parser #3689
Conversation
naik-aakash
requested review from
JaGeo,
shyuep,
janosh and
mkhorton
as code owners
| overlaps.append(float(el)) | ||
| self.band_overlaps_dict[spin][" ".join(kpoint_array)]["matrix"].append(overlaps) | ||
| rows += [float(el)] | ||
| overlaps += [rows] |
There was a problem hiding this comment.
overlaps is possibly unbound now from renaming overlaps = [] to rows above.
we don't check for this in CI yet due to way too many legacy violations but really should. related discussion in #3646
There was a problem hiding this comment.
Hi @janosh, I did not get what you meant here. Can you please elaborate a bit more? I need the overlaps variable initialized in the previous if clause to properly store matrices for each k-point.
Is there anything I need to do ? I could not think of any other way to do this.
There was a problem hiding this comment.
the problem is, if the code enters the else case overlaps won't be declared, so Python will throw a NameError
There was a problem hiding this comment.
I think, such case won't happen as per the current file format, it will always pass through the elif clause .
But I Will check for a few more examples that I have and see If I encounter any errors
There was a problem hiding this comment.
Hi @janosh , I checked for about 20 different example files which I have, does not result in NameError due to file format , logic seems to work without breaking it 😃
But if you have any better idea to get same outcome, I am happy to implement it. Just at this point I seem to have blanked out 😅 and cannot think of any alternatives
There was a problem hiding this comment.
you could pip install pyright and run it on this file. it'll show you the error i mean. you just need to ensure overlaps is declared by all code paths
There was a problem hiding this comment.
Hi @janosh , I have now addressed the errors which I could using pyright. If you think these are okay it could be merged.
janosh
added
io
Issue
Current implementation of
Bandoverlapsparser adds keys with '.' inband_overlaps_dictattribute leading to failure of pydantic checks in atomate2 lobster workflow materialsproject/atomate2#769Changes
band_overlaps_dictattribute ofBandoverlapsclass