Lobster io improvements

[naik-aakash]

Todo

Make MSONable

• Charge
• Sitepotential
• Grosspop
• Bandoverlap
• MadelungEngeries
• Icohplist
• Update tests

[naik-aakash]

Hi @JaGeo , I made this changes as per our prep to update our schema in atomate2.

[JaGeo]

Thank you!

@naik-aakash , could you add tests for from_dict as well? And, doesn't monty already implement as_dict. Why do you need to implement this yourself?

[JaGeo]

Just to add @naik-aakash and I will discuss this and then update the PR.

[davidwaroquiers]

Hello,

If I may, here are some possibly useful comments.

I would avoid having the filename in the init, but only have the data itself and have a from_file method that reads in.
Example for Charge object:

class Charge(MSONable):
    def __init__(self, atomlist, types, mulliken, loewdin):
        self.atomlist = atomlist
        self.types= types
        self.mulliken= mulliken
        self.loewdin= loewdin

    @classmethod
    def from_file(cls, filename: str = "CHARGE.lobster"):
        with zopen(filename, mode="rt") as file:
            data = file.read().split("\n")[3:-3]
        if len(data) == 0:
            raise OSError("CHARGES file contains no data.")

        atomlist: list[str] = []
        types: list[str] = []
        mulliken: list[float] = []
        loewdin: list[float] = []
        for line in data:
            sp = line.split()
            atomlist.append(sp[1] + sp[0])
            types.append(sp[1])
            mulliken.append(float(sp[2]))
            loewdin.append(float(sp[3]))
        return cls(atomlist=atomlist, types=types, mulliken=mulliken, loewdin=loewdin)

    @property
    def num_atoms(self)
        return len(self.atomlist)

Such an organization of the object is usually cleaner than init with filenames (which are actually not exactly compatible with MSONable, which is probably why @naik-aakash needed to reimplement as_dict). For standard MSONable to work, the init method should only set the attributes and do nothing else no "post-processing" of the arguments passed to init ideally.

Then indeed you don't need to implement as_dict and from_dict in principle.

Also I would avoid having capitalized attributes (herabove mulliken instead of Mulliken), otherwise you may think it's a class.

Same applies to the other objects of course. I don't know if it's possible or easy to change for you on the packages/scripts using these but I would say it would improve the lobster io if it is possible.

Best

[JaGeo]

Thanks, @davidwaroquiers !

[JaGeo]

Some first thoughts from my side.

It would be good to keep the breaking changes minimal as these classes are used by other people. Thus, I am not sure if implementing a from_file method at this point is a good idea.

Vasp output io implements the classes similarily. And I think I took them as an example when I initially started with this development, e.g.,

pymatgen/pymatgen/io/vasp/outputs.py

Line 143 in 098b2a9

class Vasprun(MSONable):

In any case, if we implement an as_dict method, the from_dict one needs to work.

And, we can rename the property names but I would suggest to add a deprecation warning then. I agree that I did not follow naming conventions very well when I initially implemented these classes. Might take some time to correct all of these issues.

[JaGeo]

My answer sounds a bit too direct. Sorry for this. I
n any case, the suggestions are very helpful but we need to keep a balance between correcting all poor design choices from some years ago, consistency with other code parts and few breaking changes.

[davidwaroquiers]

No offense taken @JaGeo ;)
I fully agree there is a trade-of to take into account when making backward incompatible changes.
Nevertheless, I also tend to compare with other (much more used) packages, such as numpy, matplotlib, pandas, pydantic and many others, which did (recently) huge changes with big backward incompatible changes. While I agree it was (and still is for some of these changes) annoying to adapt the codes, scripts and the like, I believe these changes made sense as they were all adding value: cleaner api, more standardized usage, removal of weird (or wrong) methods/features, easier development of new features, more modularity, ...
Anyway, I'm currently not a user of lobster (nor a user or developer of lobster_io) and you should decide what's best for the lobster community.
As for the vasp io, I believe it is the same trade-off, with a larger user base. The choices made 10-15 years ago for these seemed good at the time and now we are living with these choices, which will probably not change unless a big backward incompatible refactoring happens all at once (not in any current plan of pymatgen I believe).

Best,

[naik-aakash]

Thank you @davidwaroquiers , for your suggestions. They were certainly insightful and would be nice to keep in mind when implementing new stuff here on.

[naik-aakash]

Hi @JaGeo , I have adapted Charge class now, without introducing breaking change am able to get as_dict and from_dict working out of the box without having to define it myself. If you think this is fine, will add a similar approach to other classes as well.

Note: Will add deprecation warning for renamed attributes , I just want to know if you think this approach is fine. 😃

[JaGeo]

I am fine with the changes. We will need these modifications to make the integration of the LOBSTER data into the MP API and emmet builders possible.

@janosh : in case you have time to review it. (Technically, I can merge now but will of course not do it until one of the maintainers is fine with it)

[janosh]

@JaGeo i'm happy for you to make merge decisions! no one more qualified to review this code than you! 😄

don't want to speak for @shyuep and @mkhorton but i imagine they too much appreciate your help!

[janosh]

fwiw, i had the same thought as @davidwaroquiers and was planning to make similar comments but they've already been discussed so feel free to merge

[JaGeo]

@janosh , great thanks.

Is there any io class that you would recommend to model other io classes after? Might be helpful for future implementations. I think we have very different strategies depending on the io module at the moment.

[janosh]

good question! maybe the recently added FHI-aims IO modules? e.g.

pymatgen/pymatgen/io/aims/outputs.py

Lines 30 to 50 in 0f474f6

	class AimsOutput(MSONable):
	"""The main output file for FHI-aims."""
	def __init__(
	self,
	results: Molecule | Structure | Sequence[Molecule | Structure],
	metadata: dict[str, Any],
	structure_summary: dict[str, Any],
	) -> None:
	"""AimsOutput object constructor.
	Args:
	results (Molecule or Structure or Sequence[Molecule or Structure]): A list
	of all images in an output file
	metadata (Dict[str, Any]): The metadata of the executable used to perform
	the calculation
	structure_summary (Dict[str, Any]): The summary of the starting
	atomic structure
	"""
	self._results = results
	self._metadata = metadata

the io.res module also has pretty good code quality. e.g. ResProvider and AirssProvider

pymatgen/pymatgen/io/res.py

Lines 323 to 353 in 0f474f6

	class ResProvider(MSONable):
	"""Provides access to elements of the res file in the form of familiar pymatgen objects."""
	def __init__(self, res: Res) -> None:
	"""The :func:`from_str` and :func:`from_file` methods should be used instead of constructing this directly."""
	self._res = res
	@classmethod
	def _site_spin(cls, spin: float | None) -> dict[str, float] | None:
	"""Check and return a dict with the site spin. Return None if spin is None."""
	if spin is None:
	return None
	return {"magmom": spin}
	@classmethod
	def from_str(cls, string: str) -> ResProvider:
	"""Construct a Provider from a string."""
	return cls(ResParser._parse_str(string))
	@classmethod
	def from_file(cls, filename: str) -> ResProvider:
	"""Construct a Provider from a file."""
	return cls(ResParser._parse_file(filename))
	@property
	def rems(self) -> list[str]:
	"""The full list of REM entries contained within the res file."""
	return self._res.REMS.copy()
	@property
	def lattice(self) -> Lattice:

io.vasp uses InputFile and similar classes feel a bit "boilerplatey" so i wouldn't always model after io.vasp even though it's widely used

pymatgen/pymatgen/io/core.py

Line 48 in 0f474f6

class InputFile(MSONable):