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

Dependencies: update the aiida-pseudo requirements #609

Merged
merged 1 commit into from Nov 17, 2020

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Nov 17, 2020

The new required version of aiida-pseudo allows multiple cutoff sets
to be defined on a pseudopotential family at different levels of
stringency. It also changes the CLI option to pass a pseudopotential
family to -F.

The UpfData class was also updated to now require the z_valence
value to be parsed in addition to the element, so the test fixtures are
updated to include that in the generated pseudo nodes.

The tests are adapted and a new fixture sssp is added, which for the
whole session creates an instance of SsspFamily. Since this is a
costly operation, it should only be done once and all tests should
reuse it. This means that tests can no longer blindly use the fixture
clear_database_before_tests, which anyway was not useful in terms of
efficiency.

The new required version of `aiida-pseudo` allows multiple cutoff sets
to be defined on a pseudopotential family at different levels of
stringency. It also changes the CLI option to pass a pseudopotential
family to `-F`.

The `UpfData` class was also updated to now require the `z_valence`
value to be parsed in addition to the element, so the test fixtures are
updated to include that in the generated pseudo nodes.

The tests are adapted and a new fixture `sssp` is added, which for the
whole session creates an instance of `SsspFamily`. Since this is a
costly operation, it should only be done once and all tests should
reuse it. This means that tests can no longer blindly use the fixture
`clear_database_before_tests`, which anyway was not useful in terms of
efficiency.
@sphuber sphuber requested a review from mbercx November 17, 2020 08:24
Copy link
Member

@mbercx mbercx left a comment

Choose a reason for hiding this comment

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

Thanks @sphuber! All goods good to me, just left a question with regard to file handle flushing.

with open(filename, 'w+b') as handle:
with upf.open(mode='rb') as source:
handle.write(source.read())
handle.flush()
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity: why do we flush the file handle here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just to make sure it is written to disk, but I think it is not actually necessary here, since by the time the file will be used, it is outside of this context manager, the closing of which should guarantee the flushing as well.

@sphuber sphuber merged commit 09367dc into develop Nov 17, 2020
@sphuber sphuber deleted the fix/aiida-pseudo branch November 17, 2020 13:08
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.

None yet

2 participants