Add type annotations for io.vasp.outputs

[DanielYang59]

Summary

• Add type annotations for io.vasp.outputs, a follow-up PR of Add type annotations for io.vasp.inputs/optics #3740.
• Use PathLike as type over str for all paths.
• Add missing docstrings and clean up docstrings.
• Lazily import Vasprun/Xdatcar in Trajectory to be consistent with other part of the class.

[DanielYang59]

@janosh While you are working on improving dcostrings, can you please skip this io.vasp.outputs part to avoid overlapping? I would help fixing some docstring issues while adding types. Thanks!

[DanielYang59]

I'm now facing significant resistance from the "default value as None behavior" (most mypy errors are more or less related to possible default None values), similar to #3724.

Default values for get or similar methods

For the following example:

poscar_dict = {"lattice": Lattice, "pos": Position}  # a terrible example dict

- comment: str | None = poscar_dict.get("comment", None)
+ comment: str = poscar_dict.get("comment", "")

# More downstream operations that only works for `str`
clean_comment = comment.strip()

Using empty str "" could avoid the type being expanded to str | None.

Or perhaps:

pymatgen/pymatgen/io/vasp/outputs.py

Lines 3630 to 3633 in a16ac39

	@property
	def net_magnetization(self):
	"""Net magnetization from Chgcar"""
	return np.sum(self.data["diff"]) if self.is_spin_polarized else None

Such default values as None would expand the type from float to float | None, meaning all downstream operations (or child objects) need to explicitly check for None.

Default values for functions/methods

For example

pymatgen/pymatgen/io/vasp/outputs.py

Lines 223 to 235 in a16ac39

	def __init__(
	self,
	filename: str | Path,
	ionic_step_skip: int | None = None,
	ionic_step_offset: int = 0,
	parse_dos: bool = True,
	parse_eigen: bool = True,
	parse_projected_eigen: bool = False,
	parse_potcar_file: bool = True,
	occu_tol: float = 1e-8,
	separate_spins: bool = False,
	exception_on_bad_xml: bool = True,
	) -> None:

The default ionic_step_skip is None, which would require all downstream methods to handle this case. Using a default int which will NOT normally happen during runtime could bypass this issue (for number of steps, -1 might be a good choice).

Some preliminary thoughts

• For sequence types, use an empty sequence might be quite straightforward ("" for str, []/()/{} for list/tuple/dict).
• For numbers, maybe use a number that would NOT happen during runtime (for example, -1 for number of CPU cores).

Summary

Such cases are happening everywhere across the code base (for all kinds of types) and need to insert huge amount of checks if var is None. I'm not sure how to resolve this gracefully (I cannot even describe this issue properly because it's happening everywhere and in various forms). Any suggestion would be appreciated @janosh.

[janosh]

Any suggestion would be appreciated @janosh.

definitely feel free to homogenize types where it make sense and simplifies the code. there are cases where it's important to be explicit about accepting None and handling that case separately. but if an empty string/tuple/... works for the default case, by all means use that over None