Skip to content

Commit b5440ed

Browse files
authored
Add comments for Aaron Kaplan to clarify
1 parent 3b9b5c7 commit b5440ed

File tree

1 file changed

+11
-6
lines changed

1 file changed

+11
-6
lines changed

pymatgen/io/validation/check_incar.py

+11-6
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,11 @@ def _check_incar(
3838
note that all changes to `reasons` and `warnings` can be done in-place
3939
(and hence there is no need to return those variables after every function call).
4040
Any cases where that is not done is just to make the code more readable.
41-
I didn't think that would be necessary here.
41+
# MK: A better description of this class should be included above the current docstring.
42+
# I think writing clearly how the whole function operates, in chronological order, would be best.
4243
"""
4344

44-
working_params = GetParams(
45+
working_params = GetParams( # MK: unclear
4546
parameters=parameters,
4647
defaults=_vasp_defaults,
4748
input_set=valid_input_set,
@@ -75,7 +76,7 @@ def _check_incar(
7576

7677

7778
class GetParams:
78-
"""Initialize current params and update defaults as needed."""
79+
"""Initialize current params and update defaults as needed.""" # MK: unclear, expand
7980

8081
_default_defaults = {
8182
"value": None,
@@ -101,7 +102,9 @@ def __init__(
101102
vasp_version: Sequence[int],
102103
task_type,
103104
fft_grid_tolerance: float,
104-
) -> None:
105+
) -> None:
106+
# MK: unclear. I think a thorough docstring describing how this class init
107+
# operates, in chronological order, would be best.
105108
self.parameters = copy.deepcopy(parameters)
106109
self.defaults = copy.deepcopy(defaults)
107110
self.input_set = input_set
@@ -630,6 +633,7 @@ def update_ionic_params(self):
630633

631634
class BasicValidator:
632635
"""Lightweight validator class to handle majority of parameter checking."""
636+
# MK: unclear. Is the above docstring accurate? It seems like all checks use this, right?
633637

634638
# avoiding dunder methods because these raise too many NotImplemented's
635639
operations: tuple[str, ...] = ("==", ">", ">=", "<", "<=", "in", "approx", "auto fail")
@@ -670,7 +674,8 @@ def _check_parameter(
670674

671675
# Allow for printing different tag than the one used to access values
672676
# For example, the user sets ENCUT via INCAR, but the value of ENCUT is stored
673-
# by VASP as ENMAX
677+
# by VASP as ENMAX
678+
# MK: is the above comment in the best place?
674679

675680
append_comments = append_comments or ""
676681

@@ -714,7 +719,7 @@ def check_parameter(
714719
current_values = [current_values]
715720
reference_values = [reference_values]
716721

717-
if not all(operation in self.operations for operation in operations):
722+
if not all(operation in self.operations for operation in operations): # MK: unclear
718723
# Do not validate
719724
return
720725

0 commit comments

Comments
 (0)