added lobster calc quality summary method to analyze module #115
Conversation
|
I would add a text output as well. Maybe similar to.our automatic-analysis. --json then creates a json |
Yes, I intend to add this. Just wanted if anything more you think we should add besides these keys. |
To do
|
|
Hi @JaGeo , now the text descriptions are also generated and options to save the dict data as json is also added to cli. I have attached example outputs of text generated. DOS and BVA charge comparisons are optional as well. I will add bit more descriptive text concerning Charge comparisons. If you have any suggestions, please let me know :-)
|
|
This PR can also be reviewed. I will add more tests to increase coverage when working on tests speed up to avoid adding lot of files. |
Updated text output
|
|
Hi @JaGeo , have updated the tests here to increase the coverage of cli as well. I feel this PR is ready for review now 😄 |
lobsterpy/cohp/analyze.py
Outdated
|
|
||
| mull_oxi = [] | ||
| for i in lobs_charge.Mulliken: | ||
| if i >= 0: |
There was a problem hiding this comment.
Can work but always make sure it is numerically okay. As the charges are quite large, this probably works.
There was a problem hiding this comment.
I thought about it, but could not figure out any other way to check this. If you have any suggestions let me know
|
Let me know if anything more needs to be changed or modified. 😄 |
|
New updated outputs for calc quality dict and text
|
|
@naik-aakash What happens if we only have very few points for the computation of the Tanimoto fingerprint or VASP and Lobster computations are too different? I think we really need to test a few cases here before we merge. |
lobsterpy/cli.py
Outdated
| } | ||
|
|
||
| for arg, default_value in dos_files.items(): | ||
| file_path = getattr(args, arg) |
There was a problem hiding this comment.
would be good to have some more descriptive names here.
There was a problem hiding this comment.
Made it descriptive. Also replaced the not used default_value variable in with _.
lobsterpy/cohp/analyze.py
Outdated
| quality_dict["DOS_comparisons"] = {} # type: ignore | ||
|
|
||
| for orb in dos_lobster.get_spd_dos(): | ||
| if e_range[0] >= min(dos_vasp.energies) and e_range[0] >= min( |
There was a problem hiding this comment.
Your test cases cover all these ifs?
There was a problem hiding this comment.
Updated tests and now if covers all this 😃 . Have also, update the workflow file and now we can also see missing line numbers
I do have a warning printed out for cases when too less points are there in VASP and LOBSTER DOS computations <2000 stating the DOS comparisons will not be reliable and rerun computations with higher NEDOS here is the warning |
Thank you! Could you check the text again for missing articles, please? Also, "There exists" should better be "There are". Maybe copy the text into grammarly as your PI is also not the best person for English grammar. |
|
Hi @JaGeo , currently cli test will fail as the interface would need to be updated and the I will do this tomorrow, but using your suggestion of potcar symbol lists works fine |
I have also updated the texts to fix missing articles. |
#Changes
PR for issue #30
Added a convenience static method to ensure lobster calc reliablitly. Checks implemented are similar to our Publication
The output is like this at the moment. If you have any other suggestions @JaGeo please let me know.
