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

Executable: Switch from HasHDF to HasDict #1377

Merged
merged 10 commits into from
Mar 26, 2024
Merged

Executable: Switch from HasHDF to HasDict #1377

merged 10 commits into from
Mar 26, 2024

Conversation

jan-janssen
Copy link
Member

@jan-janssen jan-janssen commented Mar 16, 2024

This pull request reverts parts of #728 to simplify the to_dict() and from_dict() interface of the GenericJob class. In the future the interface should use __getstate__() and __setstate__().

@jan-janssen jan-janssen added the format_black reformat the code using the black standard label Mar 16, 2024
@jan-janssen jan-janssen marked this pull request as draft March 16, 2024 20:41
@jan-janssen jan-janssen added format_black reformat the code using the black standard and removed format_black reformat the code using the black standard labels Mar 16, 2024
@jan-janssen jan-janssen changed the title Executable: Implement to_dict() and from_dict() Executable: Switch from HasHDF to HasDict Mar 16, 2024
@jan-janssen jan-janssen requested a review from pmrv March 16, 2024 22:06
@jan-janssen jan-janssen marked this pull request as ready for review March 16, 2024 22:06
@pmrv
Copy link
Contributor

pmrv commented Mar 26, 2024

Actually wouldn't it be much easier to just give HasStorage a HasDict implementation? That would be quite trivial, since it's just

def to_dict(self):
  return self.storage.to_builtin()
def from_dict(self, data_dict):
  self.storage.clear()
  self.storage.update(data_dict)

@@ -40,6 +43,7 @@ def __init__(
overwrite_nt_flag (bool):
"""
super().__init__()
self.storage = DataContainer()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmrv We still use the DataContainer here

Co-authored-by: Marvin Poul <poul@mpie.de>
@@ -1049,6 +1050,9 @@ def to_dict(self):
"exclude_groups_hdf": self._exclude_groups_hdf,
}
data_dict["server"] = self._server.to_dict()
self._executable_activate_mpi()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmrv The self._executable_activate_mpi() was just moved up the to the point where the to_dict() method is called on the executable object.

@jan-janssen
Copy link
Member Author

Actually wouldn't it be much easier to just give HasStorage a HasDict implementation? That would be quite trivial, since it's just

def to_dict(self):
  return self.storage.to_builtin()
def from_dict(self, data_dict):
  self.storage.clear()
  self.storage.update(data_dict)

As I commented in #1398 I do not think it is that easy but I am happy if you convince me otherwise.

@jan-janssen jan-janssen merged commit 28f7b0a into main Mar 26, 2024
24 of 25 checks passed
@jan-janssen jan-janssen deleted the executable_dict branch March 26, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black reformat the code using the black standard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants