Make `Incar` keys case insensitive, fix init `Incar` from dict val processing for str/float/int #4122

DanielYang59 · 2024-10-18T03:28:51Z

Summary

Fix Incar.check_params handling of case sensitivity, to fix VaspInput setter and Incar.check_params() are inconsistent #4119, also add unit tests
Issue warning if duplicated keys found when init Incar

Make more operations (get/update/setdefault) in `Incar` case insensitive

Init Incar from dict (also the from_dict method) doesn't seem to use setter method, might be better off subclass UserDict
Update Incar class docstring to record the change in behaviour
Test other dict methods like update/setdefault/...

Init `Incar` from dict doesn't call setter method, leading to unprocessed str/float/int value

Setter method only trigger proc_val when val is string, leading to inconsistent behaviour when init from dict for int/float
Setter method not triggered on update method when subclassing builtin dict, leading to val not passing through proc_val when init from dict

DanielYang59 · 2024-10-18T03:52:36Z

ping the true expert @esoteric-ephemera in case you have any comments (perhaps deserve a separate PR):

~~Do we want to cast INCAR keys to upper case (to be consistent with VASP behaviour, but potentially breaking)~~
Do we want to automatically filter duplicated INCAR keys (with a warning of course), again to be consistent with VASP behaviour (this should be resolved by casting keys to upper, as we cannot have duplicated keys in dict)
Wondering why do we capitalize values for unknown string keys?

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

Line 988 in 32d9beb

return val.strip().capitalize()

Thanks in advance :)

This reverts commit adcdba7.

esoteric-ephemera · 2024-10-18T15:48:43Z

Do we want to cast INCAR keys to upper case (to be consistent with VASP behaviour, but potentially breaking)

Why do this exactly? FORTRAN (unless you use specific compiler options) is case insensitive, and most FORTRAN code, VASP included, is case insensitive. aLgO = aLL should mean the same thing as ALGO = ALL to VASP

Do we want to automatically filter duplicated INCAR keys (with a warning of course), again to be consistent with VASP behaviour (this should be resolved by casting keys to upper, as we cannot have duplicated keys in dict)

I would just throw a user warning. Because of how VASP parses an INCAR, I'm pretty sure you can write the same tag multiple times, and the bottom-most value is the one that's accepted

Wondering why do we capitalize values for unknown string keys?

Probably just convention, again because FORTRAN is case insensitive

Also for the check_incar stuff: keep in mind that FORTRAN is very strictly typed. Even if I write ENCUT = 500 in the INCAR, fortran will enforce this to be double precision because ENCUT is declared as a double

JaGeo · 2024-10-18T15:52:03Z

I btw recently had issues with updating the incar. I would have expected that sigma and SIGMA would simply modify SIGMA but they did not. Instead, i had two different sigmas in the incar. I did not have time to investigate but it was a very recent pymatgen version

esoteric-ephemera · 2024-10-18T15:53:45Z

@JaGeo did VASP parse them as different variables or just take the one defined furthest down the INCAR?

shyuep · 2024-10-18T15:55:53Z

Yes, all tags are case insensitive. I don't think the Incar class actually checks. So if you set sigma and SIGMA, you end up with two sigma tags. The easiest solution is to automatically capitalize the key in setitem for Incar. I would use Camelcase for the values for Incar (even though it makes no difference where VASP is concerned). Usually VASP just looks at the first letter. E.g., ALGO = Accurate and ALGO=A means the same thing.

DanielYang59 · 2024-10-18T15:56:08Z

Probably just convention, again because FORTRAN is case insensitive

Thanks that makes sense, and I don't think we need to change this as it wouldn't have any side effect.

I'm pretty sure you can write the same tag multiple times, and the bottom-most value is the one that's accepted

did VASP parse them as different variables or just take the one defined furthest down the INCAR?

I believe VASP parses the first occurrence (can you confirm it's the bottom one? @esoteric-ephemera ), but I could be wrong here. I tested with INCAR:

encut = 500
ENCUT = 400

And grep ENCUT vasprun.xml would return: <i name="ENCUT"> 500.00000000</i>

JaGeo · 2024-10-18T16:01:01Z

@DanielYang59 @esoteric-ephemera Yes, it took the first one as far as I remember.

@shyuep i might be wrong but I thought i modelled the lobsterin class after the incar at the time and i think it checked at some point. In any case, it would be great if this would be checked again (e.g., via Camelcase).
EDIT: ah, already implemented 😀

DanielYang59 · 2024-10-18T16:05:16Z

Why do this exactly?

I don't think the Incar class actually checks. So if you set sigma and SIGMA, you end up with two sigma tags. The easiest solution is to automatically capitalize the key in setitem for Incar.

I proposed that we cast Incar keys to upper case internally in order to prevent such duplicate and make other internal checking/handling easier.

Also the reason I personally prefer upper case over others (capitalize/lower) being:

Most (if not all) our internal recordings (incar_parameters.json, and all kinds of recordings inside vasp.inputs class) use upper case, so we don't need to update them all
VASP internally cast all keys to upper case when parsing (see the top comment in VASP source code drdatab.F)
VASP wiki also use upper case, so I assume it's more or less a convention?

shyuep · 2024-10-18T16:18:48Z

@JaGeo I just fix the upper case keys problem and added a test. It should no longer be a problem. As far as I can tell, the values are already properly capitalize and there is a test.

DanielYang59 · 2024-10-19T01:56:51Z

Just mention this in case you have any comments, it seems the ML_MODE tag (which I never used) is an exception of the case insensitivity, added in #3625:

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

Line 932 in ed8542d

lower_str_keys = ("ML_MODE",)

Per the above VASP wiki:

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.

DanielYang59 · 2024-10-19T03:33:11Z

src/pymatgen/io/vasp/inputs.py

@@ -709,10 +710,12 @@ class BadPoscarWarning(UserWarning):
    """Warning class for bad POSCAR entries."""


-class Incar(dict, MSONable):
+class Incar(UserDict, MSONable):


Subclassing UserDict would make our life much easier, otherwise some methods like update/setdefault would not call the overridden setter method, and I believe that's the recommended way to create a custom dict:

UserDict: wrapper around dictionary objects for easier dict subclassing

Similar to our previous discussion on Lobsterin which is also a case-insensitive dict:

pymatgen/src/pymatgen/io/lobster/inputs.py

Line 56 in ed8542d

class Lobsterin(UserDict, MSONable):

Perhaps we should consider writing a reusable key case-insensitive dict at some point instead of writing this over and over again.

Or use (now that we do have requests as a dependency):

from requests.structures import CaseInsensitiveDict

DanielYang59 · 2024-10-19T04:39:20Z

src/pymatgen/io/vasp/inputs.py

-        )
+        key = key.strip().upper()
+        # Cast float/int to str and feed into proc_val to clean up their types
+        val = self.proc_val(key, str(val)) if isinstance(val, str | float | int) else val


This change:

- val = self.proc_val(key, str(val)) if isinstance(val, str) else val + val = self.proc_val(key, str(val)) if isinstance(val, str | float | int) else val

should have a healthy dose of stupidity as I believe casting int/float to str is necessary such that the val would go through proc_val to sanitize its type, otherwise it would bypass the proc_val check, leading to inconsistent behaviour when init from dict vs from str/file.

…ld be checked

DanielYang59 · 2024-10-19T06:38:58Z

src/pymatgen/io/vasp/outputs.py

@@ -345,7 +345,7 @@ def __init__(
                self.update_potcar_spec(parse_potcar_file)
                self.update_charge_from_potcar(parse_potcar_file)

-        if self.incar.get("ALGO") not in {"CHI", "BSE"} and not self.converged and self.parameters.get("IBRION") != 0:
+        if self.incar.get("ALGO") not in {"Chi", "Bse"} and not self.converged and self.parameters.get("IBRION") != 0:


Before migrating to subclassing UserDict, the setter method is not triggered through update method when init from a dict, therefore these values remain as is (BSE) when it should be capitalized (Bse).

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

Lines 718 to 737 in ed8542d

def __init__(self, params: dict[str, Any] | None = None) -> None:

"""

Create an Incar object.

Args:

params (dict): Input parameters as a dictionary.

"""

super().__init__()

if params is not None:

# If INCAR contains vector-like MAGMOMS given as a list

# of floats, convert to a list of lists

if (params.get("MAGMOM") and isinstance(params["MAGMOM"][0], int | float)) and (

params.get("LSORBIT") or params.get("LNONCOLLINEAR")

):

val = []

for idx in range(len(params["MAGMOM"]) // 3):

val.append(params["MAGMOM"][idx * 3 : (idx + 1) * 3])

params["MAGMOM"] = val

self.update(params)

DanielYang59 added 5 commits October 18, 2024 10:50

move ENCUT from int to float list

b9b3804

remove str case sensitivity in check_params

a5ba6ca

fix return type annotation

60e1a48

fix unit test failure

945368b

use original key for warning msg

f0ebb34

add duplicate check in check_params

37ac9f5

DanielYang59 force-pushed the fix-4119-incar-tag-check branch from f94d43e to 37ac9f5 Compare October 18, 2024 11:22

DanielYang59 added 5 commits October 18, 2024 19:32

add test first, issue not fixed yet

13b5a67

init from dict also use setter method and fix val filter logic

adcdba7

relocate test_from_file_and_from_dict

bae2320

Revert "init from dict also use setter method and fix val filter logic"

44a06c3

This reverts commit adcdba7.

remove seemingly unused monkeypatch

6799044

DanielYang59 force-pushed the fix-4119-incar-tag-check branch from 3ce90e8 to 82f732c Compare October 18, 2024 13:38

tweak type

966bb91

DanielYang59 force-pushed the fix-4119-incar-tag-check branch from 82f732c to 966bb91 Compare October 18, 2024 13:41

DanielYang59 added 2 commits October 18, 2024 21:46

make module level var all cap

532cad0

casting to list doesn't seem necessary, remain iterator for lazy eval

a24ac8e

DanielYang59 force-pushed the fix-4119-incar-tag-check branch from 88b5eab to a24ac8e Compare October 18, 2024 13:55

add docstring to clarify parse list

92bfc7e

shyuep added a commit that referenced this pull request Oct 18, 2024

Capitalize INCAR keys by default. #4122

042bea3

Merge branch 'master' into fix-4119-incar-tag-check

f1fb45b

DanielYang59 added 4 commits October 19, 2024 09:38

remove duplicate check

536eae5

reduce indentation level

ba0f349

remove docstring of warn that doesn't exist

198ce83

fix typo in incar tag ECUT -> ENCUT

a2da974

inherit from UserDict, and make more ops case insensitive

435320d

DanielYang59 changed the title ~~Fix Incar.check_params handling of case sensitivity~~ Fix Incar.check_params handling of case sensitivity, make Incar keys case insensitive Oct 19, 2024

DanielYang59 commented Oct 19, 2024

View reviewed changes

DanielYang59 added 2 commits October 19, 2024 11:39

also override del and in methdos

51f5922

issue warning for duplicate keys

3b231be

DanielYang59 force-pushed the fix-4119-incar-tag-check branch from d43bb1a to 3b231be Compare October 19, 2024 03:54

DanielYang59 added 4 commits October 19, 2024 12:04

enhance warning check

dbaaa98

tweak docstring

f8b223c

fix type of float/int casting

f30e740

enhance test for from_dict consistency check

259124d

DanielYang59 commented Oct 19, 2024

View reviewed changes

relocate duplicate check to setter so that both from str and dict wou…

96ad945

…ld be checked

DanielYang59 force-pushed the fix-4119-incar-tag-check branch from 27ad437 to 96ad945 Compare October 19, 2024 05:39

fix index error for vasprun

fa58d0d

DanielYang59 force-pushed the fix-4119-incar-tag-check branch from ac2a40a to fa58d0d Compare October 19, 2024 06:35

DanielYang59 commented Oct 19, 2024

View reviewed changes

DanielYang59 added 2 commits October 19, 2024 14:49

move duplicate warning to init otherwise get false pos when update

8d5768e

enhance unit test from type cast from dict

c350714

DanielYang59 marked this pull request as ready for review October 19, 2024 07:05

DanielYang59 requested review from shyuep and mkhorton as code owners October 19, 2024 07:05

DanielYang59 added 3 commits October 19, 2024 15:37

remove unnecessary get default

48d2755

remove unnecessary type cast in check_params

b2e9a63

tweak Incar docstring

9dc98f9

DanielYang59 changed the title ~~Fix Incar.check_params handling of case sensitivity, make Incar keys case insensitive~~ Make Incar keys case insensitive, fix init Incar from dict val processing for str/float/int Oct 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Incar` keys case insensitive, fix init `Incar` from dict val processing for str/float/int #4122

Make `Incar` keys case insensitive, fix init `Incar` from dict val processing for str/float/int #4122

DanielYang59 commented Oct 18, 2024 •

edited

Loading

DanielYang59 commented Oct 18, 2024 •

edited

Loading

esoteric-ephemera commented Oct 18, 2024 •

edited

Loading

JaGeo commented Oct 18, 2024

esoteric-ephemera commented Oct 18, 2024

shyuep commented Oct 18, 2024

DanielYang59 commented Oct 18, 2024 •

edited

Loading

JaGeo commented Oct 18, 2024 •

edited

Loading

DanielYang59 commented Oct 18, 2024 •

edited

Loading

shyuep commented Oct 18, 2024

DanielYang59 commented Oct 19, 2024 •

edited

Loading

DanielYang59 Oct 19, 2024 •

edited

Loading

DanielYang59 Oct 19, 2024 •

edited

Loading

DanielYang59 Oct 19, 2024 •

edited

Loading

	def __init__(self, params: dict[str, Any] \| None = None) -> None:
	"""
	Create an Incar object.

	Args:
	params (dict): Input parameters as a dictionary.
	"""
	super().__init__()
	if params is not None:
	# If INCAR contains vector-like MAGMOMS given as a list
	# of floats, convert to a list of lists
	if (params.get("MAGMOM") and isinstance(params["MAGMOM"][0], int \| float)) and (
	params.get("LSORBIT") or params.get("LNONCOLLINEAR")
	):
	val = []
	for idx in range(len(params["MAGMOM"]) // 3):
	val.append(params["MAGMOM"][idx * 3 : (idx + 1) * 3])
	params["MAGMOM"] = val

	self.update(params)

Make Incar keys case insensitive, fix init Incar from dict val processing for str/float/int #4122

Are you sure you want to change the base?

Make Incar keys case insensitive, fix init Incar from dict val processing for str/float/int #4122

Conversation

DanielYang59 commented Oct 18, 2024 • edited Loading

Summary

Make more operations (get/update/setdefault) in Incar case insensitive

Init Incar from dict doesn't call setter method, leading to unprocessed str/float/int value

DanielYang59 commented Oct 18, 2024 • edited Loading

esoteric-ephemera commented Oct 18, 2024 • edited Loading

JaGeo commented Oct 18, 2024

esoteric-ephemera commented Oct 18, 2024

shyuep commented Oct 18, 2024

DanielYang59 commented Oct 18, 2024 • edited Loading

JaGeo commented Oct 18, 2024 • edited Loading

DanielYang59 commented Oct 18, 2024 • edited Loading

shyuep commented Oct 18, 2024

DanielYang59 commented Oct 19, 2024 • edited Loading

DanielYang59 Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

DanielYang59 Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

DanielYang59 Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Make `Incar` keys case insensitive, fix init `Incar` from dict val processing for str/float/int #4122

Make `Incar` keys case insensitive, fix init `Incar` from dict val processing for str/float/int #4122

DanielYang59 commented Oct 18, 2024 •

edited

Loading

Make more operations (get/update/setdefault) in `Incar` case insensitive

Init `Incar` from dict doesn't call setter method, leading to unprocessed str/float/int value

DanielYang59 commented Oct 18, 2024 •

edited

Loading

esoteric-ephemera commented Oct 18, 2024 •

edited

Loading

DanielYang59 commented Oct 18, 2024 •

edited

Loading

JaGeo commented Oct 18, 2024 •

edited

Loading

DanielYang59 commented Oct 18, 2024 •

edited

Loading

DanielYang59 commented Oct 19, 2024 •

edited

Loading

DanielYang59 Oct 19, 2024 •

edited

Loading

DanielYang59 Oct 19, 2024 •

edited

Loading

DanielYang59 Oct 19, 2024 •

edited

Loading