Clearly define public vs. private interface

[aiidateam]

Clearly define public vs. private interface

[greschd]

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

?

[giovannipizzi]

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 : public
    • cpinputparser
      • CpInputFile : public, directly under parsers OK
    • pwinputparser:
      • PwInputFile : public, directly under parsers OK
      • parse_k_points: private
    • qeinputparser:
      • QeInputFile : public, directly under parsers OK
      • get_cell_from_parameters: private
      • get_structure_from_qeinput: private
      • parse_atomic_positions: private
      • parse_atomic_species: private
      • parse_cell_parameters: private
      • parse_namelists: private
      • str2val: private
      • strip_comment: private
  • generators : public
    • cell_conversion : private
      • get_parameters_from_cell : public (directly under generators)
  • utils:
    • _qe_version: private
      • parse_version: private
    • exceptions: move to top-level qe_tools.exceptions, make exceptions public
  • constants: keep and make public

What do you think? Happy for different suggestions of course

[giovannipizzi]

Also, for renaming, some suggestions:

• maybe we can rename 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?
• 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

[greschd]

Thanks 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.

[sphuber]

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.

[greschd]

If you would like my 2 centimes,

always appreciated 👍

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.

[sphuber]

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.

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

[giovannipizzi]

Let's go with this