Improve INCAR tag check

[DanielYang59]

Summary

Some legacy from #3539.

• Clean up INCAR parameters recording file incar_parameters.json to separate types and values.
• Clean up INCAR checking mechanism accordingly.

TODOs

Enable checking of more complex type annotations like LATTICE_CONSTRAINTS : Sequence[bool, bool, bool]) suggested by @esoteric-ephemera in #3539 (comment).

[DanielYang59]

I did some research and found typeguard seems to be what we're looking for.
For example:

from typeguard import check_type, CollectionCheckStrategy


check_type(
    value=[1, "hi"], expected_type=list[int],
    collection_check_strategy=CollectionCheckStrategy.ALL_ITEMS
)

check_type(
    value=[1, "hi"], expected_type=list[int, int],
    collection_check_strategy=CollectionCheckStrategy.ALL_ITEMS
)

check_type(
    value=[1, "hi"], expected_type=eval("list[int, int]"),
    collection_check_strategy=CollectionCheckStrategy.ALL_ITEMS
)

However there are still some concerns to address:

1. Is it essentially necessary to add an extra dependency typeguard to pymatgen
2. I currently don't know any better approach to cast string to object ("list" to list) than eval, but in our case the source is internal (from file incar_parameters.json), so it should not lead to any securify issues
3. type check each item might incur a performance penalty (Type checking on generic only checks first value entry in data structure  agronholm/typeguard#418 (comment)), thus we would need to discuss the necessity of this and have a switch for strict mode (check every item or just check the first item, where check first item of a collection is the default for typeguard).

[DanielYang59]

Can you give me some advice on this @janosh? Do you think it's necessary to have an item-wise type check for INCAR tags (and might need to include an extra dependency typeguard)? Or should we stick to a more general type check (list instead of list[int, int, int] for instance). Thanks for any input in advance.

[janosh]

Do you think it's necessary to have an item-wise type check for INCAR tags (and might need to include an extra dependency typeguard)?

i think adding typeguard as a dependency just for this is a tall ask. i'm guessing @shyuep will be strongly against.
unless there's a strong selling point i would also lean against

[DanielYang59]

i think adding typeguard as a dependency just for this is a tall ask. i'm guessing @shyuep will be strongly against. unless there's a strong selling point i would also lean against

Yes I was thinking the same, adding a dependency to be used only once would be too much overhead. Actually this should be questions:

• Do you think it would be necessary to perform item-wise check for INCAR tags (or just keep current more general type checking)? @janosh I'm pretty unsure and worried about the potential performance penalty here.
• If so, including an extra dependency should certainly be discouraged. I would see if I could find another way around.

[janosh]

Do you think it would be necessary to perform item-wise check for INCAR tags

i don't think so. i don't remember a lot of GitHub issues about this.

[DanielYang59]

Relocated Warning class after its corresponding class for consistency.

[DanielYang59]

i don't think so. i don't remember a lot of GitHub issues about this.

Let's keep the type checking unchanged then, and implement more careful checking when necessary. This PR should be good for review now @janosh . Thanks a lot!

[DanielYang59]

Guess this PR get forgotten... Can you please review this @janosh ? Thanks.

[janosh]

thanks @DanielYang59! lgtm

[DanielYang59]

Thanks for reviewing @janosh