-
Notifications
You must be signed in to change notification settings - Fork 79
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
Remove parser information from output_parameters #597
Conversation
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 @mbercx . Changes look good, I was just wondering if we really need this key parser_info
at all. First thing I though was to change the content to the entry point name of the parser, which then also makes it machine readable. But then I thought that this is not necessary as this value is already stored as an attribute of the CalcJobNode
itself, so we would merely be duplicating it in the output_parameters
output node. I think we can just get rid of the parser info key entirely.
I was also considering this, and I agree! In this case the |
To be honest, if you look at how the |
I've noticed that the More generally, why do we have a list of aiida-quantumespresso/aiida_quantumespresso/parsers/parse_raw/neb.py Lines 54 to 55 in 7039365
by if not finished_run: # I try to parse it as much as possible
logs.warning.append('Error while parsing the output file') of course, after creating a logging container at the start of the method with: logs = get_logging_container() |
The reason is historical. The original implementation of the parsers used |
I'm happy to work on cleaning up the parsers, but going through some of these warnings, I find a lot more than just aiida-quantumespresso/aiida_quantumespresso/parsers/parse_raw/neb.py Lines 45 to 48 in 6d4c706
aiida-quantumespresso/aiida_quantumespresso/parsers/neb.py Lines 70 to 71 in 6d4c706
There is also the I'd rather split this work up for the different parsers, and familiarize myself with the output of these calculations when I work on this, so I understand better what is going wrong, i.e. understand the meaning of all the exit codes and output parameters. For this PR, I'll just remove all the |
I actually agree that doing what I proposed is beyond the scope of this PR. I wouldn't even start to introduce the logging container in the other parsers. Let's here just simply get rid of the |
Yeah, after working on it a bit, I realize doing this partially also will just create more of a mess. I'll do as you suggest and open some Issues for cleaning up/use logging for the various parsers. |
Now that aiida-core adds both the AiiDA and plugin version to the CalcJob node attributes, as well as the parser name, it is no longer necessary to add this information to the output_parameters dictionary. This PR removes the get_parser_info() function that generates this output. The `parser_warnings` are also removed for the base/pw/ph parsers, since they were not used here. We leave the `parser_warnings` for the neb/cp parsers, but these should be replaced in a future PR by the use of a logging container.
732fb5f
to
434fff5
Compare
I think you are done with the changes here, correct @mbercx ? If so I will review it |
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.
Looks good to me, thanks @mbercx
Now that `aiida-core` adds both the AiiDA and plugin version to the `CalcJobNode` attributes, as well as the parser name, it is no longer necessary to add this information to the output_parameters dictionary. This PR removes the `get_parser_info()` function that generates this output. The `parser_warnings` are also removed for the base/pw/ph parsers, since they were not used here. We leave the `parser_warnings` for the neb/cp parsers, but these should be replaced in a future PR by the use of a logging container.
Fixes #582
Now that aiida-core adds both the AiiDA and plugin version to the
CalcJob node attributes, as well as the parser name, it is no longer
necessary to add this information to the output_parameters dictionary.
This PR removes the get_parser_info() function that generates this
output.
The
parser_warnings
are also removed for the base/pw/ph parsers, sincethey were not used here. We leave the
parser_warnings
for the neb/cpparsers, but these should be replaced in a future PR by the use of a
logging container.