Closed greschd closed 4 years ago
Here is a possible suggestion, adapting from yours, with the philosophy that we can make methods public easily later if we want to, while the reverse is complex for backwards incompatibility:
qe_tools
: public
parsers
: publiccpinputparser
CpInputFile
: public, directly under parsers
OKpwinputparser
:
PwInputFile
: public, directly under parsers
OKparse_k_points
: privateqeinputparser
:
QeInputFile
: public, directly under parsers
OKget_cell_from_parameters
: privateget_structure_from_qeinput
: privateparse_atomic_positions
: privateparse_atomic_species
: privateparse_cell_parameters
: privateparse_namelists
: privatestr2val
: privatestrip_comment
: privategenerators
: publiccell_conversion
: private
get_parameters_from_cell
: public (directly under generators
)utils
:_qe_version
: private
parse_version
: privateexceptions
: move to top-level qe_tools.exceptions, make exceptions publicconstants
: keep and make publicWhat do you think? Happy for different suggestions of course
Also, for renaming, some suggestions:
qe_tools.parsers.cpinputparser
to e.g. qe_tools.inputparsers.cp
or something like this?get_structure_from_qeinput
-> shall we change the name to something simpler? is get_structure
clear enough? or parse_structure
? or should we put an underscore between qe
an input
?__init__
of QeInputFile, probably it doesn't make sense that it's called pwinput
. Maybe simply input_file
or input_file_content
or input_content
? We might want to do a similar discussion more generally once we agree on which are the public methodsThanks a lot! I think I agree on the public / private suggestions (at least, I didn't see anything that I didn't agree with at first glance).
* maybe we can rename `qe_tools.parsers.cpinputparser` to e.g. `qe_tools.inputparsers.cp` or something like this?
Yeah, that makes sense. Is output-parsing also within the scope of this library? If so, we could consider either a structure
parsers
input
output
generators
input
or flat:
input_parsers
(or inputparsers
)output_parsers
(currently doesn't exist)input_generators
I tend to favor the flat structure, although that means right now the options are just input_parsers
and input_generators
- which seems slightly redundant, but actually IMO nicer than the somewhat confusing generators
.
* `get_structure_from_qeinput` -> shall we change the name to something simpler? is `get_structure` clear enough? or `parse_structure`? or should we put an underscore between `qe` an `input`?
Definitely. I think this opens up a separate discussion, see #42
* shall we rename the kwargs of the public methods/functions, where it might make sense? E.g. for the `__init__` of QeInputFile, probably it doesn't make sense that it's called `pwinput`. Maybe simply `input_file` or `input_file_content` or `input_content`? We might want to do a similar discussion more generally once we agree on which are the public methods
Yeah, input_content
or just content
seems good to me.
If you would like my 2 centimes, I would prefer the nested structure, were it not for the fact that you will run into the problem when importing from input
you will get "keyword reserved" issues. So maybe the flat one works best here. Although I would definitely keep the underscore in that case.
If you would like my 2 centimes,
always appreciated :+1:
you will run into the problem when importing from
input
you will get "keyword reserved" issues.
Ha, I hadn't even considered that. Though it would technically work because input
is a function, not a keyword - but shadowing a builtin is rarely a great idea.
What's still bugging me here is that get_parameters_from_cell
actually doesn't really generate the input - not to the point where you could write it to disk, at least. So maybe input_converters
would be a better name there. If we think it could also be used on output, bare converters
could also work.
We could also ditch the "input" from the module name completely, and have the classes be importable as e.g. qe_tools.parsers.PwInputFile
.
What's still bugging me here is that
get_parameters_from_cell
actually doesn't really generate the input - not to the point where you could write it to disk, at least. So maybeinput_converters
would be a better name there.
This makes me think that the actual "generators" are currently embedded in the CalcJob
plugins of aiida-quantumespresso
. It might be useful to look if we can factor that out and include here. This would make it a lot easier for consumers that do not want this mingled with AiiDA. The QE input generator tool on the materials cloud comes to mind. This now has to install aiida-quantumespresso
just to build input file from some python values. We already factored this code out in aiida-quantumespresso
to even make this possible
Absolutely agree - for now we can just design the naming here to make sure it fits when we move that over. How about
parsers
generators
converters
?
Let's go with this
Ahead of the
2.0.0
release, I think we should clearly define public vs. private interface:__all__
defined__all__
in the module where it is defined.ipython
should autocomplete only the public parts whenc.IPCompleter.limit_to__all__ = True
is set.I can do the changes to the code, but the important question is: which parts should be in the public interface? We have:
qe_tools
: publicparsers
: publiccpinputparser
: ?CpInputFile
: public, directly underparsers
pwinputparser
: ?PwInputFile
: public, directly underparsers
parse_k_points
: ?qeinputparser
: ?QeInputFile
: public, directly underparsers
get_cell_from_parameters
: ?get_structure_from_qeinput
: ?parse_atomic_positions
: ?parse_atomic_species
: ?parse_cell_parameters
: ?parse_namelists
: ?str2val
: ?strip_comment
: ?generators
: publiccell_conversion
: ?get_parameters_from_cell
: public (here, or directly undergenerators
)?utils
: ?_qe_version
: privateparse_version
: privateexceptions
: ?constants
: ?In
aiida-quantumespresso
, the following are used:qe_tools.constants
qe_tools.parsers
,CpInputFile
andPwInputFile
are used in code, their "real" location (e.g.qe_tools.parsers.cpinputparser.CpInputFile
is used in docstringsqe_tools.utils.exceptions.ParsingError
is used in testing@giovannipizzi @sphuber @lekah what's your opinion? If needed, now would also be a good time to move/rename things.