Skip to content
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 handling for ibrav=91/-13 to 'cell_from_parameters'. #35

Merged
merged 2 commits into from
May 26, 2020

Conversation

greschd
Copy link
Member

@greschd greschd commented May 13, 2020

Fixes #28.

Because the meaning of ibrav=-13 changed in QE version 6.4.1, this adds a way of dealing with different QE versions:

The parse_version function turns a string (e.g. '6.4.1') into a packaging.version.Version. By default the latest version is used, which is implemented as a singleton object that compares as greater than any Version object.

In the interface, the version can be specified as the optional keyword-only argument qe_version. Because this option needs to be available both at the top-level QeInputFile.__init__ and on the free functions (e.g. get_cell_from_parameters), it accepts either a string or an already-parsed version. This allows immediately parsing the version in QeInputFile.__init__, while also accepting string input in get_cell_from_parameters.

The way this is implemented is by making the parse_version idempotent, meaning that it will not change an already-parsed version.

In the tests, the qe_version can be passed as a keyword to singletest. However, these tests currently cannot auto-generate the reference file.

Dependency changes:

  • Uses pytest as a test runner (haven't modified the existing tests yet, but these could potentially be simplified a lot with pytest-regressions)
  • Adds packaging as an explicit dependency. Since this is part of the standard Python tooling (via setuptools) it should usually be installed already anyway.

To be reviewed especially carefully:

  • The logic for ibrav = 91/-13 matches the pw.x input description
  • The public interface change of adding the qe_version keyword-only argument. The other changes (adding parse_version etc.) are internal-only.

@greschd greschd changed the title [BLOCKED] Add handling for 'ibrav=91' to 'cell_from_parameters'. [BLOCKED] Add handling for ibrav=91/-13 to 'cell_from_parameters'. May 13, 2020
@greschd greschd force-pushed the greschd/issue28 branch 3 times, most recently from 350fd09 to bc8216e Compare May 14, 2020 17:46
@greschd greschd changed the title [BLOCKED] Add handling for ibrav=91/-13 to 'cell_from_parameters'. Add handling for ibrav=91/-13 to 'cell_from_parameters'. May 14, 2020
Dominik Gresch added 2 commits May 14, 2020 23:29
Also fixes the '--write-ref' option in the tests, which was broken
in aiidateam#33 because it used the 'filename' interface of the parser
__init__.
Because the meaning of ibrav=-13 changed in QE version 6.4.1,
this adds a way of dealing with different QE versions. The
'parse_version' function turns a string (e.g. '6.4.1') into
a 'packaging.version.Version'. By default the latest version is
used, which is implemented as a singleton object that compares
as greater than any 'Version' object.
In the interface, the version can be specified as the optional
keyword-only argument 'qe_version'. Because this option needs to
be available both at the top-level __init__ of the 'QeInputFile'
and on the free functions (e.g. 'get_cell_from_parameters'), it
accepts either a string or an already-parsed version. This allows
immediately parsing the version in 'QeInputFile.__init__', while
also accepting string input in 'get_cell_from_parameters'.
The way this is implemented is by making the 'parse_version'
idempotent, meaning that it will not change an already-parsed
version.

In the tests, the 'qe_version' can be passed as a keyword to
'singletest'. However, these tests currently cannot auto-generate
the reference file.
Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I checked the docs and the results of the tests, and I think the current output matches what's written in the QE docs.

Also, I understand correctly that the new parameter is anyway optional, right? In this case I see no problem.

@greschd
Copy link
Member Author

greschd commented May 16, 2020

Yes, qe_version is optional - it will fall back on the newest version we know about.

@giovannipizzi giovannipizzi merged commit fdb5ff5 into aiidateam:develop May 26, 2020
@greschd greschd deleted the greschd/issue28 branch May 26, 2020 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing 'valid_ibrav' values in get_cell_from_parameters
2 participants