Pyiron job for Extra FIM simulations #8
Conversation
changed the sum_singl_k function to work with **kwargs of path
There was a problem hiding this comment.
overall, looks very good, but I have minor suggestions to improve code clarity
General remarks:
- imports should use relative locations (leading . or ..)
- there is code duplication between job classes, I think we can avoid this by moving this to standalone functions that we call from both classes.
- functions for internal use should begin with underscore, to make clear that they shouldn't be called by the user
- hdf5 writing for partial results shouldn't be done by sum_single_k directly; splitting compute and output writing will make a much clearer control flow.
| if 'path' in kwargs: | ||
| filename = kwargs['path']+'/'+f"partial_dos{ik}.h5" | ||
| else: | ||
| filename = f"partial_dos{ik}.h5" | ||
| with h5py.File(filename, "w") as handle: | ||
| handle.create_dataset( | ||
| "ionization_energies", data=self.inputDict["ionization_energies"] |
There was a problem hiding this comment.
I personally think that sum_single_k should not write a file, but rather return the computed partial result. File writing could be handled outside, e.g. as part of the job that executes the sum_single_k function. This would give a much cleaner code organization.
| __date__ = "March 5, 2024" | ||
|
|
||
|
|
||
| class ExtraFimSimulatorRefJob(TemplateJob): |
There was a problem hiding this comment.
For my understanding: this is the single-k computing job class, no? Should we reflect this in the class name?
There was a problem hiding this comment.
essentially it is but, the user is not going to access this job. It is a reference job for the actual job.
EXTRA_FIM/pyiron_job.py
Outdated
| self.ref_job = ExtraFimSimulatorRefJob(project=project, job_name=job_name) | ||
|
|
||
| def extrpolate_potential(self): | ||
| """Extrapolate potential if needed, to extrapolate waves to higher distances""" |
There was a problem hiding this comment.
This looks like a direct copy of the function body further up. Is it possible to refactor such that we have a single implementation for both cases?
EXTRA_FIM/pyiron_job.py
Outdated
| @property | ||
| def suggest_input_dict(self): | ||
| """Suggests a input dictionary based on the electrostatic potential, Fermi and ionization energy and populates input""" |
There was a problem hiding this comment.
Same as above:
- should be a function, not a property, I think
- can we avoid body duplication?
| @@ -0,0 +1,26 @@ | |||
| [tool.poetry] | |||
| name = "extra-fim" | |||
| version = "0.1.0" | |||
There was a problem hiding this comment.
When will we bump this to 1.0?
There was a problem hiding this comment.
May be once we are ready to publish this as a package. Either on pypi or conda forge. Then
relative imports Co-authored-by: freyso <63301887+freyso@users.noreply.github.com>
relative imports Co-authored-by: freyso <63301887+freyso@users.noreply.github.com>
relative imports Co-authored-by: freyso <63301887+freyso@users.noreply.github.com>
relative imports Co-authored-by: freyso <63301887+freyso@users.noreply.github.com>
relative imports Co-authored-by: freyso <63301887+freyso@users.noreply.github.com>
typo Co-authored-by: freyso <63301887+freyso@users.noreply.github.com>
|
I will work on the rest of the comments and once, they are all in, I will let you people know. ignore the commits meanwhile. |
This version of the code can perform a pyiron job for extra FIM simulations. Please excuse the number of commits, as I had to work both on cluster and locally.
The pyiron job is based on the parallelmaster job class. The main job creates sub jobs based on the number of k points, and automatically collects the output to the main job.
I have also added a jupyter notebook detailing the workflow for pyiron job.
I also had to make small changes to the main code, mostly adding correct import orders, as I tried to make the code into an installable package.
Of course, this is a preliminary version. I would be happy to make changes and enhance the functionality of pyiron job if needed.