Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read INCAR SYSTEM as is, check_params use proc_val #4136

Merged
merged 14 commits into from
Nov 12, 2024
Merged

Read INCAR SYSTEM as is, check_params use proc_val #4136

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
057f6af
remove unnecessary processing
DanielYang59 Oct 26, 2024
82b59de
stupid manual update of GGA recording
DanielYang59 Oct 26, 2024
60eb627
stupid manually add ZAB_VDW tag
DanielYang59 Oct 26, 2024
1b4438a
avoid casing SYSTEM value
DanielYang59 Oct 26, 2024
633db73
fix SYSTEM case and add test
DanielYang59 Oct 26, 2024
b3d92a8
tweak comment
DanielYang59 Oct 26, 2024
826a27c
use a non capitalized string for test
DanielYang59 Oct 26, 2024
eab2e2a
fix test string
DanielYang59 Oct 26, 2024
d0fa08a
revert str compare, startswith is slower
DanielYang59 Oct 26, 2024
fdfed3a
Merge remote-tracking branch 'upstream/master' into vasp-eq-ignore-co…
DanielYang59 Oct 27, 2024
b53d52c
add test for Incar pop method
DanielYang59 Oct 27, 2024
c365099
check_params consider lower str exception
DanielYang59 Oct 27, 2024
b89da9d
reuse proc_val in check_params, thanks @yantar92
DanielYang59 Oct 27, 2024
64a3888
Merge branch 'master' into vasp-eq-ignore-comment
DanielYang59 Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions src/pymatgen/io/vasp/incar_parameters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,24 +191,35 @@
"GGA": {
"type": "str",
"values": [
"91",
"PE",
"AM",
"HL",
"CA",
"MK",
"RE",
"VW",
"B3",
"PZ",
"SL",
"CA_C",
"PZ_C",
"VW",
"HL",
"WI",
"LIBXC",
"LI",
"91",
"PE",
"PBE_X",
"PBE_C",
"RE",
"RP",
"B5",
"BF",
"CO",
"PS",
"AM",
"B3",
"B5",
"OR",
"BO",
"MK",
"ML",
"CX",
"BF",
"RP",
"CO",
"RE",
"03",
"05",
"10",
Expand Down Expand Up @@ -1444,5 +1455,8 @@
},
"MAGDIPOLOUT": {
"type": "bool"
},
"ZAB_VDW": {
"type": "float"
}
}
8 changes: 4 additions & 4 deletions src/pymatgen/io/vasp/outputs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ def _parse(
or elem.attrib["comment"]
== "INVERSE MACROSCOPIC DIELECTRIC TENSOR (including local field effects in RPA (Hartree))"
):
if self.incar.get("ALGO", "Normal").upper() == "BSE":
if self.incar.get("ALGO", "Normal") == "Bse":
Copy link
Contributor Author

@DanielYang59 DanielYang59 Oct 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #4122 we should be confident that string values are now capitalized (except for those in lower_str_keys and as_is_str_keys), these case conversion may not be needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they are capitalized, why comparing with "Bse" (non-capitalized)

Copy link
Contributor Author

@DanielYang59 DanielYang59 Oct 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I may not understand your question? The value Bse is capitalized? The key ALGO is in uppercase.

print("BSE".capitalize())  # Bse

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sory, I got completely confused. Mostly because of apparent inconsistency between the code and VASP wiki. If you look at https://www.vasp.at/wiki/index.php/ALGO, you will see that BSE and many other values are all capitals:

ALGO = Normal | VeryFast | Fast | Conjugate | All | Damped | Subrot | Eigenval | Exact | None | Nothing | CHI | G0W0 | GW0 | GW | scGW0 | scGW | G0W0R | GW0R | GWR | scGW0R | scGWR | ACFDT | RPA | ACFDTR | RPAR | BSE | TDHF

So, the code reads awkward now.

Copy link
Contributor Author

@DanielYang59 DanielYang59 Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got completely confused. Mostly because of apparent inconsistency between the code and VASP wiki

Yes I find it confusing too #4122 (comment), and @esoteric-ephemera said it's a convention to pymatgen #4122 (comment)

This is likely like this since the start:
image

As VASP itself is largely case insensitive, we just need to internally standardize for easy handling (no need to cast to upper/capitalize when comparing values and so on). I personally prefer to cast both value and tag to uppercase for the following reason:

  • VASP Wiki large prefer upper case for keys, and get rid of the headache to memorize "key is uppercase", but "case is capitalized" and so on
  • Most values (just like keys) are first-letter abbreviations of some sort, capitalizing them doesn't make much sense, though there're values that is indeed proper words like "Fast/Conjugate/All/..."

But this would be badly breaking (every downstream package would find the INCAR key in a different case) so I guess it's not worth the price. So we'd better come up with something that is not breaking. In any case I would leave this for a future PR.

I'm not sure it's doable or not, a good alternative is to subclass a "case insensitive string" as an inplace replacement of built in str to avoid the headache altogether (but I currently don't have a good way to overwrite the membership check), I might have another try later.

class CaseInsensitiveStr(str):
    def __new__(cls, value):
        return super().__new__(cls, value.casefold())

    def __eq__(self, other):
        if isinstance(other, str):
            return super().__eq__(other.casefold())
        return NotImplemented

    def __hash__(self):
        return hash(self.casefold())

str_a = CaseInsensitiveStr("HELLO")

print(str_a)  # hello

print(str_a == "hello")  # True
print(str_a == "HELLO")  # True

print(str_a in {"hello", "world"})  # True
print(str_a in {"HELLO", "world"})  # False, should be True
print(str_a in {"world"})  # False

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I find it confusing too #4122 (comment), and @esoteric-ephemera said it's a convention to pymatgen #4122 (comment)

I see, and I agree that abstracting things out into case-sensitive/insensitive class is the most robust way to go.

As for the convention, it annoyingly has exceptions:

class Incar(UserDict, MSONable):
    """
    A case-insensitive dictionary to read/write INCAR files with additional helper functions.

    - Keys are stored in uppercase to allow case-insensitive access (set, get, del, update, setdefault).
    - String values are capitalized by default, except for keys specified
        in the `lower_str_keys` of the `proc_val` method.
    """

As you can see, capitalization is documented, except "keys specified in lower_str_keys". Currently it is a single key - ML_MODE.

However, do note that Incar.check_params does not expect such exceptions:

            if allowed_values is not None:
                # Note: param_type could be a Union type, e.g. "str | bool"
                if "str" in param_type:
                    allowed_values = [item.capitalize() if isinstance(item, str) else item for item in allowed_values]

(aside: incar_parameters.json does not even list ML_MODE)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see, capitalization is documented, except "keys specified in lower_str_keys". Currently it is a single key - ML_MODE.

Yep I updated the docstring to reflect this key/value case issue, but I'm not sure about the casing of ML_MODE either (I thought VASP is largely case insensitive ) #4122 (comment)

I never use that tag so cannot comment, it would be great if you (or someone else) could confirm this behaviour.

However, do note that Incar.check_params does not expect such exceptions:

Thanks I would update it (one exception makes everything hard to handle)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never use that tag so cannot comment, it would be great if you (or someone else) could confirm this behaviour.

I do not use this tag either, but clearly its value is case-sensitive - #3625
I looked up https://www.vasp.at/wiki/index.php/ML_MODE and it says

Warning: If any option other than the above is chosen or there is a spelling error (be careful to write everything in upper case or lower case letters) the code will exit with an error.

I conclude that case-insensitivity is not universal.

Ironically, simply upcasing (rathen than capitalizing) everything would not require making an exception for ML_MODE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not use this tag either, but clearly its value is case-sensitive - #3625

Yes I did see this PR and the VASP comment as well.

I conclude that case-insensitivity is not universal.
Ironically, simply upcasing (rathen than capitalizing) everything would not require making an exception for ML_MODE.

I believe ML_MODE is the only exception for now. upcase everything (except for SYSTEM) would make everything easy, but I guess ML_MODE was not there when this part of code was written (12 years ago), and it's hard to change it now without breakage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t's hard to change it now without breakage

100% agree.

self.dielectric_data["freq_dependent"] = self._parse_diel(elem)
elif "density" not in self.dielectric_data:
self.dielectric_data["density"] = self._parse_diel(elem)
Expand Down Expand Up @@ -641,7 +641,7 @@ def converged_electronic(self) -> bool:
while set(final_elec_steps[idx]) == to_check:
idx += 1
return idx + 1 != self.parameters["NELM"]
if self.incar.get("ALGO", "").upper() == "EXACT" and self.incar.get("NELM") == 1:
if self.incar.get("ALGO", "") == "Exact" and self.incar.get("NELM") == 1:
return True
return len(final_elec_steps) < self.parameters["NELM"]

Expand Down Expand Up @@ -800,10 +800,10 @@ def run_type(self) -> str:
"--",
"None",
}:
incar_tag = self.incar.get("METAGGA", "").strip().upper()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would be covered by

pymatgen/src/pymatgen/io/vasp/inputs.py

Line 1029 in 6992aee

return val.strip().capitalize()

incar_tag = self.incar.get("METAGGA", "").upper()
run_type = METAGGA_TYPES.get(incar_tag, incar_tag)
elif self.parameters.get("GGA"):
incar_tag = self.parameters.get("GGA", "").strip().upper()
incar_tag = self.parameters.get("GGA", "").upper()
run_type = GGA_TYPES.get(incar_tag, incar_tag)
elif self.potcar_symbols[0].split()[0] == "PAW":
run_type = "LDA"
Expand Down