-
Notifications
You must be signed in to change notification settings - Fork 80
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
PhParser: allow for pattern initialization #1034
base: main
Are you sure you want to change the base?
Conversation
The ph.x should be parallelized by setting in the input parameters `start_irr` and `last_irr` to 0. This allows the program to exit smoothly and it further avoids to wait for the rewriting of the wavefunctions, which can be a rather long and intensive IO operation, not really suited for initialization runs. The parser is then adjusted to account for this option, as for some reason the line having `JOB DONE` is not printed in such cases. A simple specialized parser is also added to store the number of q-points and their values, which can be later on used to parallelize over q-points by specifying `last_q` and `start_q`.
4df9fb0
to
5c396c1
Compare
if parameters: | ||
self.out('output_parameters', orm.Dict(parameters)) | ||
return |
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.
In the case that parameters
is empty, wouldn't that reasonably correspond to some kind of error? Or are you intentionally letting it continue the parsing in that case to find a generic error?
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.
Yeah in principle ph.x can still throw some errors, say if something was wrong with some files etc.
q_points = [list(map(float, coord)) for coord in coords] | ||
|
||
parameters.update({'q_points': q_points}) |
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.
Would it make sense perhaps to return this as an actual KpointsData
instead of a 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.
Yes, I thought about that, maybe it would. On the other hand, I was even wondering whether it's worth it to actually parse it or not. If one has a grid, one can/should use start/last_q
, if you provide a KpointsData, this would return the same node basically. So, don't know. Any strong opinion? What about maybe adding some "post-process" parsing via tools
?
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 am not quite sure I fully understand the use case of this initialization run. But if for the common use case a user would actually want to use the parsed grid as an input for the next calculation (i.e. they are going to turn it into a KpointsData
anyway) then we might as well have the parser do it here.
If, instead, the kpoints won't be used as is, but in parts and so the KpointsData
would have to be transformed, then you might as well just leave it as is.
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.
This initialization run is the proper initialization for ph.x, which avoids the use of the .EXIT file, which would still make ph.x to rewrite the wavefunctions for nothing (hence, wasting time - to give an idea, an 18 atoms system it would take ~20 min, which are wasted node hours). The key ingredient here is just to determine the number of q points, and the next runs would be parallelized not with the specific q point but using start_q
and last_q
instead. The parsing of the q points as either dictionary in the output parameters or as kpointsdata is just out of completeness, but not really meant to be used (at least, as I am thinking to use this initialization run). I could simply remove it at this point.
if parameters['number_of_qpoints'] != len(parameters['q_points']): | ||
return parameters, False | ||
|
||
return parameters, True |
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 am not a big fan of communicating errors through return values, especially not if that means having to turn the return value into a tuple. Since you only really use the parameters
result in case there isn't a problem, what is the problem with just raising an exception and catching that in the caller?
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 @bastonero . Just the one test is failing. If you fix that, I will merge
The ph.x should be parallelized by setting in the input parameters
start_irr
andlast_irr
to 0. This allows the program to exit smoothly and it further avoids to wait for the rewriting of the wavefunctions, which can be a rather long and intensive IO operation, not really suited for initialization runs.The parser is then adjusted to account for this option, as for some reason the line having
JOB DONE
is not printed in such cases. A simple specialized parser is also added to store the number of q-points and their values, which can be later on used to parallelize over q-points by specifyinglast_q
andstart_q
.PS: this also avoids the creation of the
aiida.EXIT
file, which triggers the program to stop.Note: this is also the recommended way from the official webpage. To report the statements in case the link will be broken in the future: