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

Update template job to use ~~DataContainer~~ HasStorage #428

Merged
merged 4 commits into from
Sep 14, 2021

Conversation

liamhuber
Copy link
Member

@JNmpi here it is. I noticed also that this job type is not available to create, but is only accessible with from pyiron_base import TemplateJob. My gut feeling is that this is mostly a parent class and not actually instantiated, so it's OK that it doesn't show up on the creation list?

@jan-janssen sorry to pester you for review, but the only place this might have an impact is over in pyiron_experimental for pystem, temmeta, and matchseries. For the first two I can already see there's not going to be an impact, but matchseries is more complicated and it has no tests. Since you're the sole contributor there I'd appreciate if you'd quickly look over and/or set up a test.

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.

Suggested a gotcha in to/from_hdf. GenericJob.to_hdf/from_hdf both set self._hdf5 aka self.project_hdf to hdf.open(group_name) if group_name is defined. It's therefore sufficient to point self._data to self.project_hdf5 and leave group_name to the default table_name. If that is not done, you'd end up writing both the job and its input in the same HDF5 group.

pyiron_base/job/template.py Outdated Show resolved Hide resolved
pyiron_base/job/template.py Outdated Show resolved Hide resolved
@liamhuber liamhuber changed the title Update template job to use DataContainer Update template job to use ~~DataContainer~~ HasStorage Sep 9, 2021
@liamhuber liamhuber requested a review from pmrv September 9, 2021 14:09
@liamhuber
Copy link
Member Author

@pmrv I think I can actually dodge the gotcha altogether by leveraging one of your other fine creations ;)

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.

lgtm now!

@liamhuber liamhuber merged commit 5bbdd53 into master Sep 14, 2021
@delete-merged-branch delete-merged-branch bot deleted the modern_template_syntax branch September 14, 2021 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants