New class to handle NcICOBILIST.lobster files
#2878
Conversation
QuantumChemist
changed the title
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
I think the errors here should (mostly) disappear if you pull in |
The failing checks are caused by my implementation of the wrong format for the pytests. I will correct that within the next weeks and then I hope this can be merged, even though for now certain NcICOBILIST.lobster outputs should be handled with some caution. |
|
Hi @janosh :D |
QuantumChemist
changed the title
|
Could you do an empty commit to get another run of the tests, @QuantumChemist ?
|
I had to commit some changes anyways :) |
.attach_pid50466
Outdated
There was a problem hiding this comment.
oopsie, sorry that was accidental. I created a mess when I merged the two versions
There was a problem hiding this comment.
I removed it now :)
pymatgen/io/lobster/outputs.py
Outdated
| @@ -408,6 +409,113 @@ def icohpcollection(self): | |||
| return self._icohpcollection | |||
|
|
|||
|
|
|||
| class Ncicobilist: | |||
There was a problem hiding this comment.
Why not pascal case NciCobiList here? Easier to read imo.
There was a problem hiding this comment.
Here I wanted to stick to only capitalizing the first letter like in class Icohplist
For me NciCobilist is not very readable as the method referring to is NcICOBI, so maybe we can go with NcIcobilist or ncIcobilist ?
There was a problem hiding this comment.
Hi @janosh, please let me know what you think about this. If it's not ok, I'm gonna adopt your suggestion.
There was a problem hiding this comment.
I still prefer consistency with the rest of pymatgen which uses PascalCase for all class names. E.g. we have PotcarSingle and not POTCARSingle for that reason. But maybe another maintainer wants to chime in here.
There was a problem hiding this comment.
(It does not have to be consistent with the older Lobster class names.)
There was a problem hiding this comment.
@janosh, ok it's done :)))
test_files/cohp/NcICOBILIST.lobster
Outdated
There was a problem hiding this comment.
Just to confirm, are new test files strictly necessary here? And if so, can they be compressed?
There are some that look like they could potentially be re-used:
$ ls tests/files/** | grep COBILIST
ICOBILIST.lobster
ICOBILIST.lobster.additional_case
ICOBILIST.lobster.spinpolarized
ICOBILIST.lobster.spinpolarized.additional_case
ICOBILIST.lobster.withoutorbitalsThere was a problem hiding this comment.
The files mentioned here are not the ones I need. Seems that they haven't been fetched in the merge process. I gonna upload them quickly.
There was a problem hiding this comment.
Oh now I see with the merging process they have just been added to test_files/cohp (so like it was structured before). I gonna fix that now
There was a problem hiding this comment.
so the files in question are
$ ls tests/files/** | grep NcICOBI
NcICOBILIST.lobster
NcICOBILIST.lobster.gz
NcICOBILIST.lobster.nospin
NcICOBILIST.lobster.nospin.withoutorbitals
NcICOBILIST.lobster.withoutorbitals
please let me know if you see something unnecessary. From my POV this is the least user cases that should be checked.
Thank you for helping me with this PR :)
|
|
||
| # check if orbitalwise NcICOBILIST | ||
| # include case when there is only one NcICOBI | ||
| for entry in data: # NcICOBIs orbitalwise and non-orbitalwise can be mixed | ||
| if len(data) > 2 and "s]" in str(entry.split()[3:]): | ||
| self.orbitalwise = True | ||
| warnings.warn( | ||
| "This is an orbitalwise NcICOBILIST.lobster file. Currently, the orbitalwise information is not " | ||
| "read!" | ||
| "This is an orbitalwise NcICOBILIST.lobster file. " |
There was a problem hiding this comment.
Is it possible to get orbital-wise interactions for multicenter ICOBIs?
There was a problem hiding this comment.
Yes, it is and the output format can be very complex with more than 3 centers or mix of n center interactions.
There was a problem hiding this comment.
Also when Peter and I implemented the COBI, we never really thought of a high-throughput application, so the NcICOBIs are ordered as specified in the lobsterin... mixing different n center interactions up, therefore results in a very messed up NcICOBILIST.lobster file, especially when some are orbitalwise and other are not 🙈 need to check if and how that changed for lobster version 5 before implementing an orbitalwise feature here.
There was a problem hiding this comment.
Thanks @QuantumChemist. Left some minor comments about naming and failing test.
pymatgen/io/lobster/outputs.py
Outdated
| """ | ||
|
|
||
| if filename is None: | ||
| warnings.warn("Please consider using the newest LOBSTER version (4.1.0+). See http://www.cohp.de/.") |
There was a problem hiding this comment.
Is this warning correctly indented? Why only warn if filename is None?
There was a problem hiding this comment.
It looks like a leftover from the icohplist. I remember implementing this (with another if clause). I guess this can be removed.
There was a problem hiding this comment.
Wouldn't you still use the default naming option in such a case and then then the file wouldn't be there? Thus, shouldn't it be the warning if the default file name does not exist rather than when it is set to None?
pymatgen/io/lobster/outputs.py
Outdated
| # include case when there is only one NcICOBI | ||
| for entry in data: # NcICOBIs orbitalwise and non-orbitalwise can be mixed | ||
| if len(data) > 2 and "s]" in str(entry.split()[3:]): | ||
| self.orbitalwise = True |
There was a problem hiding this comment.
Is self.orbitalwise missing a default value of False? Tests are currently failing with
AttributeError: 'NciCobiList' object has no attribute 'orbitalwise'
There was a problem hiding this comment.
Oh, I have to look more into this... :O
pymatgen/io/lobster/outputs.py
Outdated
| ) | ||
| break # condition has only to be met once | ||
|
|
||
| if self.orbitalwise: |
There was a problem hiding this comment.
Maybe rename to self.orbital_wise for readability?
pymatgen/io/lobster/outputs.py
Outdated
| # TODO: add functions to get orbital resolved NcICOBIs | ||
|
|
||
| @property | ||
| def ncicobilist(self) -> dict[Any, dict[str, Any]]: |
There was a problem hiding this comment.
Maybe rename to def ncicobi_list()?
…on check COBI features were only implemented in LOBSTER 4.1.0
Co-authored-by: J. George <[email protected]> Signed-off-by: Christina Ertural <[email protected]>
|
Hi @janosh :D |
janosh
changed the title
NcICOBILIST.lobster files
janosh
added
enhancement
|
yay 🎉 thank you very much for your help @janosh! |
Summary
Implemented a class to handle multi-center ICOBIs via NcICBILIST.lobster files with pymatgen in
pymatgen/io/lobster/outputs.pyand updated some comments.Ncicobilistis similar toIcohplistbut not identical because of the different format styles of NcICOBILIST.lobster and ICO**LIST.lobster output files.Added the respective NcICOBI test files to
test/files/cohpfor non-spinpolarized, non-orbitalwise case etc.Todo (if any)
Checking LOBSTER version (done)
Adapt for orbitalwise case. (will leave that out for now)