-
Notifications
You must be signed in to change notification settings - Fork 78
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
PP plugin, add back 'settings' input #537
Conversation
c400834
to
e22859d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yakutovicha , implementation looks fine. Could you maybe just add a test for the cmdline
setting?
If you update the branch, the tests should now also run because I pushed a fix to |
e22859d
to
fa22b7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yakutovicha . Three minor things and then this is good to go
tests/calculations/test_pp.py
Outdated
@@ -24,6 +24,8 @@ def _generate_inputs(parameters=None): | |||
generate_remote_data(fixture_localhost, fixture_sandbox.abspath, 'quantumespresso.pw'), | |||
'parameters': | |||
orm.Dict(dict=parameters), | |||
'settings': | |||
orm.Dict(dict=settings) or orm.Dict(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this works as expected. The or
won't be applied to the value of settings
but orm.Dict(dict=settings)
, so no matter the value of settings
you will always hit the first clause. Since orm.Dict(dict=None)
is fine and is exactly equal to orm.Dict()
you might as well get rid of or orm.Dict()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, didn't know. And you are right, empty AiiDA Dict
object is not falsy:
In [3]: if Dict(dict={}):
...: print("Hello")
...:
Hello
Maybe one should fix this here as well:
'settings': orm.Dict(dict=settings) or orm.Dict(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, please do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, just a side note:
In [6]: Dict(dict=None) == Dict(dict=None)
Out[6]: False
not sure whether this is what we expect.
Co-authored-by: Sebastiaan Huber <mail@sphuber.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yakutovicha
fixes #536