[WIP] Refactoring to include outputs parsing #80
Conversation
mbercx
force-pushed
the
develop
branch
3 times, most recently
from
6d45efa to
35a667f
Compare
…s. Made many functions as public since they could be useful to users and outside of their context.
…onversions should take place. In all other files, ase and pymatgen should not be used.
…ibility regarding the number of output files for each type of computation, e.g. NEB.
Several small changes are made to the `PwOutput` class: 1. Use `pathlib` instead of `os.path` for path definitions. 2. `.from_dir` method: Only look for files from the specified directory using list comprehension. 3. Adapt approach for identifying the `stdout` file to only use the first 5 lines to look for the `Program PWSCF` string. 4. Remove the try-except loop that simply catches a general exception and raises a `ValueError` instead. This would obscure the actual error without an immediate benefit. 5. Define the `d_out_xxx` variables as empty dictionaries before parsing, and redefine them in case the data is parsed successfully. 6. Allow the user to pass a `Path` instead of a string to specify the directory to parse from.
Currently the `PwStdoutParser` nor its parent class `BaseOutputFileParser` parse any of the Quantum ESPRESSO `stdout`. Here we add the `BaseStdoutParser` class that has some very minimal parsing features for parsing the code name and version, as well as the walltime of the calculation. A utility function is also added to convert the Quantum ESPRESSO walltime output into seconds. The constructor of the `BaseOutputFileParser` is also adapted to remove the default to `None`. Since there is currently no way of providing the output content after construction, it should be provided at that time for the class to be useful.
The first schema used for the XML output of Quantum ESPRESSO `pw.x` was still missing. Here it is added, along with a set of tests for the XML parsing of default runs.
|
@gbrunin thanks again for the hard work in restructuring the package. I've added some basic Now I'm adding some basic documentation, also on the design decisions. We should consider these carefully, particularly the UX, since it's hard to change these afterwards. I don't care too much about breaking backwards compatibility if we can converge on a more user-friendly tool. |
|
Also quick shoutout to @elinscott and his package for QE inputs: https://github.com/elinscott/qe_input_prototype He's already indicated he'd be fine with us integrating that work into |
|
@gbrunin I've also added some basic documentation and design notes. I'm still not quite sure about some of the APIs. I do like the general design now though, I'm just also wondering if:
I've also made a little image to represent the design:
|
…ec to utils. Renamed BaseStdoutParser.parse_stdout_base to parse for compatibility with parent abstract class.
This currently serves no real purpose, as the executable can be easily identified from the class itself. In case we do find a use case later, it can always be added again easily.
| 'numpy', | ||
| 'scipy', | ||
| 'packaging', | ||
| 'xmlschema', | ||
| ] |
There was a problem hiding this comment.
Are all these necessary ? I think it would be best to restrict to the strict minimum. I would say that for parsing or generating input even numpy should not be needed (let alone scipy ?). I'd be in favor of removing them but I don't know where/why they are used. For the packaging dependency, it has itself no dependency so I think it's probably fine to keep it even though it's probably possible to get around without it.. For xmlschema, I would also say it seems fine as xmlschema has just one dependency itself, elementpath, which itself has no dependency. Any thoughts on numpy/scipy removal ?
There was a problem hiding this comment.
Just looked, scipy is used only to get the norm of a few vectors in one file. I think this is definitely something that should then probably be removed from the dependencies (also from the fact that if numpy is kept as a required dependency, the norm can also be obtained by numpy, but for numpy, my comment below holds too :)).
As for numpy, it's used in a few more places, but 1/ for very simple maths, vector operations and 2/ for some conversion functions. For the latter point, maybe the "lowest-level" qe tools could be installed without it (the "client" code, be it aiida, pymatgen or else) has to follow the exact API that the lowest level functions require. And numpy could be an optional dependency for "activating" helper functions if the user wants it. Any thoughts ?
I have changed the structure of the code to better reflect the distinction between input and output parsers. Concerning the inputs, all the tests that were already there still pass, it's only a matter of adapting the namespace. For the outputs, I created objects such as
PwOutput, that inherit from an abstractBaseOutput, that for now can be instantiated with afrom_dirmethod (afrom_filesmethod could/should be added as well). A user with a job that ran in a given directory could get the outputs easily using this classmethod. In thesefrom_dirmethods, specific XML and/or standard outputParserswould be used to get the results. EachParserwould parse a single file, and the logic of parsing and extracting the outputs from the different codes would have to be implemented in eachfrom_dirmethod. For instance, aNebOutput.from_dirwould parse the standard output of the global computation and probably the standard outputs and/or XML files for each image. The extracted outputs would be stored as a simple dictionary and these objects would not rely on any external package.Then, in
qe_tools.extractors, ASE and pymatgen objects could be constructed (e.g., ase.Atoms/pymatgen.Structure, band structures,...), allowing each to be optional dependencies.This is only the base logic of the new structure and many things remain to be implemented. The idea now is to see what breaks with this in aiida-quantumespresso and how the parsing could be moved from there to here. Then, more will be added depending on the needs.