Missing 'valid_ibrav' values in get_cell_from_parameters

[greschd]

The get_cell_from_parameters function (link) defines the valid ibrav values as list(range(15)) + [-3, -5, -9, -12].

Comparing this to the pw.x input description, this is missing the values -13 and 91. The -13 value could be a bit problematic to implement, because the input description states:

 IMPORTANT NOTICE: until QE v.6.4.1, axis for ibrav=-13 had a
 different definition: v1(old) = v2(now), v2(old) = -v1(now)

[giovannipizzi]

91 is ok to add. -13, I don't know... As this was inconsistent, I would catch this case and raise an explicit exception explaining why we don't support it? Maybe we can then add a custom optional parameter that, if specified, can tell the function "I am aware of this, and I want to the old/new version of the definition". We need to see how to made this extensible in the future if the same happens to other ibrav numbers, and how this needs to be propagates so it's easily accessible to users when using the parser class (maybe a class property defined in the init, and then forwarded to this function?)

Also, in this case, the explicit exception message could say which flag to use to enable parsing.

[greschd]

Throwing an exception for -13 with an explanation is certainly a good start.

Maybe a good way to handle this consistently throughout would be to define a "QE version" object that can be passed into the parsers and free functions? If we encounter some logic that depends on the version, it needs to be specified otherwise an exception will be raised.

One potential issue: what happens to code that used to be independent of the QE version, but then a new QE version changes it. The above logic would turn previously-running code into an exception. So maybe it would be better to then fall back to the oldest version, with a UserWarning / DeprecationWarning.

[giovannipizzi]

I would err on the safe side, and in those cases we release a new version of qe-tools that raises. In the long term it will be strange to use the oldest version, and it will be hard to decide when to jump.

[greschd]

Makes sense :+1: