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.

Sign up for GitHub

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

Jump to bottom

GenericParameters: add to_dict() and from_dict() #1249

Merged
merged 21 commits into from
Dec 21, 2023
Merged

GenericParameters: add to_dict() and from_dict() #1249

merged 21 commits into from
Dec 21, 2023

Conversation

jan-janssen
Copy link
Member

@jan-janssen jan-janssen commented Dec 16, 2023 •
edited
Loading

In analogy to #1248 this pull request adds the to_dict() and from_dict() function to the GenericParameters class. This is required to enable more flexible storage backends.

@jan-janssen jan-janssen added the format_black reformat the code using the black standard label Dec 16, 2023
@jan-janssen jan-janssen mentioned this pull request Dec 17, 2023
Closed
@jan-janssen jan-janssen changed the title GenericParameters update to_hdf() routine GenericParameters: add to_dict() and from_dict() Dec 20, 2023
@jan-janssen jan-janssen assigned pmrv and unassigned pmrv Dec 20, 2023
@jan-janssen jan-janssen requested a review from pmrv December 20, 2023 08:48
@jan-janssen jan-janssen mentioned this pull request Dec 20, 2023
Merged
pmrv
pmrv requested changes Dec 20, 2023
View reviewed changes
pyiron_base/storage/parameters.py Outdated
@@ -936,17 +943,19 @@ def _lines_to_dict(self, lines):
lst["Comment"].append("")
return lst

def _type_to_hdf(self, hdf):
def _type_to_dict(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is one of the reasons I keep insisting on a common mixin class for to_dict/from_dict. If you had this it could be predefined automatically, instead you had to add this now manually here, in #1260 and in #1250 (#1248 should have also had it).

Copy link
Contributor

Choose a reason for hiding this comment

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

(now that it's done, go ahead with this now anyway, just making my point.)

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 I added an interface class, maybe you can have another look tomorrow.

pyiron_base/storage/parameters.py Outdated Show resolved Hide resolved
@jan-janssen jan-janssen marked this pull request as draft December 20, 2023 16:02
@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 Dec 20, 2023
@jan-janssen jan-janssen marked this pull request as ready for review December 20, 2023 19:57
pmrv
pmrv approved these changes Dec 21, 2023
View reviewed changes
Copy link
Contributor

@pmrv pmrv left a comment

Choose a reason for hiding this comment

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

Just nits, otherwise lgtm.

pyiron_base/interfaces/has_dict.py Outdated Show resolved Hide resolved
pyiron_base/interfaces/has_dict.py Outdated Show resolved Hide resolved
pyiron_base/interfaces/has_hdf.py Outdated Show resolved Hide resolved
@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 Dec 21, 2023
@jan-janssen jan-janssen merged commit 8de5ce4 into main Dec 21, 2023
22 of 23 checks passed
@delete-merged-branch delete-merged-branch bot deleted the generic_parameters branch December 21, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmrv pmrv pmrv approved these changes

Assignees
No one assigned
Labels
format_black reformat the code using the black standard
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jan-janssen @pmrv @pyiron-runner