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

Read PHONON version number from stdout #699

Closed
wants to merge 1 commit into from

Conversation

atztogo
Copy link
Contributor

@atztogo atztogo commented May 25, 2021

PR #633 is critical. To merge PR #633, @sphuber suggested to check version number, but PHONON parameter output doesn't contain the version information.

This PR is made to read the version of PHONON. The code is taken from parse_output_base method. Here I follow parse_output_base's naming of the key 'code_version', but this may be inconsistent with 'creator_version' in PW, though I don't know what exactly means 'creator'.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

Thanks! @atztogo
The implementation is good, but you should run tests/parsers/test_ph.py with --force-regen to count this new attribute.

As for 'creator_version' I believe it is the same as the code_version, while 'creator_version' only parsed in pw with XML format. Let's see what @sphuber says.

@atztogo
Copy link
Contributor Author

atztogo commented May 25, 2021

Thanks @unkcpz. I could not succeed to run the test. I simply don't understand the testing manner in aiida-quantumespresso. I looked into the documentation, but didn't find a good introduction (https://aiida-quantumespresso.readthedocs.io/en/latest/devel_guide/parsertests.html#running-tests). I would like you to include something like this PR to your PR #633 by hand (i.e., copy and paste etc) if you are fine. Then I will close this PR.

@sphuber
Copy link
Contributor

sphuber commented May 25, 2021

Thanks @atztogo . I am updating the PR of @unkcpz with the necessary info and then merging it. We can keep this open and I can add a quick test for you. No need to close it.

@sphuber
Copy link
Contributor

sphuber commented Jul 12, 2022

I came back to this PR, but I cannot actually find in #633 where I would have called for parsing the version? It is not actually being used in the code anywhere for checks. So I think we can close this. Thanks nonetheless @atztogo !

@sphuber sphuber closed this Jul 12, 2022
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.

3 participants