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
Add ShengBTE IO Functions #1518
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Rees,
Lots of nice work here. Good job with the tests. However, I think this class needs a rethink. Can you reimplement based on my code review comments.
pymatgen/io/shengbte.py
Outdated
self.file_writer_helper_func(new_dict, filename=filename) | ||
|
||
|
||
def main(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this
pymatgen/io/shengbte.py
Outdated
__date__ = "June 27, 2019" | ||
|
||
|
||
class ShengBTE_CONTROL_IO: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, rename this to Control
. The fact that it is in the shengbte module means you don't need the shengbte part. Also, we only capitalise the first letter of each word in classes.
I think this could be better written as a class which:
- Takes several dictionaries in the constructor. One for each section of the file: e.g., flags, parameters, crystal, etc. The constructor then sets the defaults if they are required.
- Has a static
from_file
method. Which just takes a file path as input. - Has a static
from_dict
method, which takes a fully formatted nested dictionary as input. - Has a (not static)
to_file
method, which writes the file and takes a file path as input and writes the settings by accessing them throughself
.
pymatgen/io/shengbte.py
Outdated
del sdict['parameters']['t_step'] | ||
return sdict | ||
|
||
def file_writer_helper_func(self, dict, filename): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused, can you not just use the f90ml writing methods instead of doing all of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using the f90ml writing methods but it returned a format of namelists that ShengBTE couldn't read
pymatgen/io/shengbte.py
Outdated
params_dict = sdict['parameters'] | ||
flags_dict = sdict['flags'] | ||
|
||
return cls(alloc_dict, crystal_dict, params_dict, flags_dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to return Control(...)
.
pymatgen/io/shengbte.py
Outdated
except: | ||
flags_dict = {} | ||
|
||
return cls(alloc_dict, crystal_dict, params_dict, flags_dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, remove the reference to cls.
Quick note. Any factory functions should be class methods. This ensures that if you just inherit from them the factory function generates the inherited class. |
@mkhorton This PR is no longer WIP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final cleanup comments and then I think this is ready.
pymatgen/io/shengbte.py
Outdated
if param not in self.as_dict(): | ||
warnings.warn( | ||
"Required parameter '{}' not specified!".format(param)) | ||
raise AttributeError("param {} not specified".format(param)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line. We already give a warning. If a user wants to make an invalid ShengBTE input file they should be able to.
pymatgen/io/shengbte.py
Outdated
|
||
|
||
class Control(dict, MSONable): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this blank line
pymatgen/io/shengbte.py
Outdated
""" | ||
return cls(**sdict) | ||
|
||
def to_file(self, filename): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you set the filename default to "CONTROL"
Summary
@utf
Initial commit for interfacing Pymatgen with ShengBTE. More features will be added later.
ShengBTE is a software package for solving the Boltzmann Transport Equation for phonons. It is capable of using 2nd and 3rd order interatomic force constants to calculate lattice thermal conductivities of bulk crystalline solids and nanowires. Full details can be found at the following link: https://doi.org/10.1016/j.cpc.2014.02.015
Added an IO for reading/writing/updating ShengBTE CONTROL files with Python dictionaries.
Additional dependencies introduced (if any)
'f90nml' for reading FORTRAN namelists used as input for running ShengBTE.