materialsproject / pymatgen

Python Materials Genomics (pymatgen) is a robust materials analysis code that defines classes for structures and molecules with support for many electronic structure codes. It powers the Materials Project.
https://pymatgen.org
Other
1.51k stars 864 forks source link

[Feature Request]: Clear warning if Bader is run without the AECCARs as reference #3448

Closed Andrew-S-Rosen closed 12 months ago

Andrew-S-Rosen commented 1 year ago

Problem

In pymatgen, it's possible to run Bader both with and without the AECCARs as reference. However, in the latter case, the results of a Bader analysis can be severely inaccurate. I don't have statistics off-hand, but U had discussed this with someone in Kristin's group (can't remember who... @janosh?) where we had a few systems with completely bizarre charges that were entirely resolved when using the AECCARs as reference. Anyway, it might be worth making this clearer because many people seem to not be aware of this and run Bader with just the CHGCAR even when they have valuable AECCARs lying around...

In bader_analysis_from_path, a warning is already raised to address this:

https://github.com/materialsproject/pymatgen/blob/f15f4b231668970c827964601b77def64b392af0/pymatgen/command_line/bader_caller.py#L460-L464

However, if someone is using the BaderAnalysis class, there's no similar information in the docstring or source code itself.

Proposed Solution

Either log a warning or (if that's too intrusive), make it much clearer in the docstring.

It should be pretty easy to address this, and I'm happy to open the PR for it, but I wanted to use the issue report to ask what people suggest the ideal approach should be.

CC @chiang-yuan.

Alternatives

No response

JaGeo commented 1 year ago

I think it's a good idea to do this for such an obvious error. However, you cannot prevent everything. If you run this after a very low precision computation in VASP, the results are probably also not reliable.

Andrew-S-Rosen commented 1 year ago

It's true we can't catch everything. However, in this case, the kwargs for AECCAR files is optional so one might not realize how important it is to include them.

janosh commented 12 months ago

but U had discussed this with someone in Kristin's group (can't remember who... @janosh?)

Yes, that was us. :)

Just noticed in BaderAnalysis.from_path we're issuing the missing POTCAR warning when actually what's missing is the AECCAR0/2 and vice versa.

https://github.com/materialsproject/pymatgen/blob/2ccbfa16cbffb1016501c18117e70a3f96a83de2/pymatgen/command_line/bader_caller.py#L396-L401

janosh commented 12 months ago

@Andrew-S-Rosen Could you clarify where we should add another warning (since bader_analysis_from_path and BaderAnalysis.from_path both already warn)?

Andrew-S-Rosen commented 12 months ago

Ah, I see that the logic does seem to suggest a warning is being raised. Apologies --- I had assumed there were no warnings based on the fact that several people have not known to use AECCARs when they had them. Feel free to close.

Andrew-S-Rosen commented 12 months ago

@janosh: I like your PR. Going to close this since I think it's just a clarification thing.