Clean up core.surface comments and docstrings

[DanielYang59]

Summary

Major clean up of core.surface, I decide to split this task into two for readability: this PR to revise comments/docstrings with some relocation of code. Another follow up PR for functional changes/fixes.

Non-breaking changes

• Major clean up of comments/docstrings (made more concise and add missing)
• Sort ordering of methods in Slab and SlabGenerator

TODOs:

• Add type annotations and format clean up in core.surface and core.structure
• Clarify wording of site and point in surface.Slab, currently a mixture of both is used and might be confusing
• Legacy fix in queen unit test from Clean up test files: dedicated VASP directories, xyz, mcif, cssr, exciting, wannier90 #3681

[janosh]

i can only assign members of MP to issues or people who've commented on issues. so if you leave a comment i could assign you. but given the 1st issue has been open since Oct 23, i don't see a high risk of other people working on it

[DanielYang59]

but given the 1st issue has been open since Oct 23, i don't see a high risk of other people working on it

I think it should be fine. People should see this PR being linked to those issues. Thanks for the help.

But I'm afraid this would take some time as it seems I would need to clean up the entire surface module

[DanielYang59]

Can you please review this? @janosh. Sorry I'm making this PR overly large, but I don't expect functional changes within this PR (mostly cleaning up of comment/docstring/namings).

And I would also expect (multiple) follow up PRs, as I did find some issues (all tagged, you could search @DanielYang59 to find tags). And I think it would be easier to view the difference if I separate them with this PR.

[coderabbitai]

Actionable comments posted: 3

[janosh]

And I would also expect (multiple) follow up PRs, as I did find some issues (all tagged, you could search @DanielYang59 to find tags). And I think it would be easier to view the difference if I separate them with this PR.

thanks a lot for polishing this module! doc string improvements are great for helping usability

[DanielYang59]

And I would also expect (multiple) follow up PRs, as I did find some issues (all tagged, you could search @DanielYang59 to find tags). And I think it would be easier to view the difference if I separate them with this PR.

thanks a lot for polishing this module! doc string improvements are great for helping usability

Thanks a lot for reviewing and capture those issues. This really took me a lot of energy but I'm now much much familiar with the module, which should hugely facilitate future functional tweaks.

[ftherrien]

@janosh @DanielYang59 I think this PR may have removed the definition of _calculate_possible_shifts or was it renamed?

[DanielYang59]

Hi @ftherrien thanks for asking. It's now renamed to gen_possible_terminations as a private function of get_slabs method under SlabGenerator class for two reasons:

• It was a private method and depended on by get_slabs alone.
• The original name is a bit misleading, because it doesn't really generate the shift but the termination (negative of shift).

pymatgen/src/pymatgen/core/surface.py

Lines 1226 to 1232 in b28c937

	def gen_possible_terminations(ftol: float) -> list[float]:
	"""Generate possible terminations by clustering z coordinates.
	Args:
	ftol (float): Threshold for fcluster to check if
	two atoms are on the same plane.
	"""

[ftherrien]

Ah! Thanks so much that makes a lot of sense

[DanielYang59]

All good! Thanks for asking and happy coding!