Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lammps input impossible to subclass nicely #1336

Open
pmrv opened this issue Feb 22, 2024 · 1 comment
Open

Lammps input impossible to subclass nicely #1336

pmrv opened this issue Feb 22, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@pmrv
Copy link
Contributor

pmrv commented Feb 22, 2024

#1271 has made it extremely inconvenient to sub class the Input class of lammps jobs. In the to_dict way of serializing objects, classes are forced to specify exactly which nodes they want to read from HDF, which forces objects to know about the nodes that their sub objects write, leading to very tight coupling.

e.g. the Input class needs to know that the control and potential objects write themselves to control_inp and potential_inp and even that the potential object requires a further potential_inp/potential. When I subclass Input now, I cannot influence these values because they are hard coded, which makes it impossible to properly implement from_dict. The only way to make saving a subclass work now is to overload to_dict add the additional data we want to write and then overload from_hdf to open the HDF file a second to read them.

I haven't looked around, but I suspect this is true for all objects that use the to/from_dict interface. This will probably be need to be fixed in base, so this is a reminder for myself.

I have a work around for now, but it is extremely annoying to rely on a half baked solution now for something that previously just worked.

@pmrv pmrv added the bug Something isn't working label Feb 22, 2024
@jan-janssen
Copy link
Member

The read_dict_from_hdf() function has an recursive parameter which loads the whole dictionary. This is already used on the from_hdf() function at the job level. So in practice the corresponding dictionaries should already be available. We should just remove all the to_hdf() and from_hdf() calls in all those sub classes and update the tests to use to_dict() and from_dict()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants