Conversation
Codecov Report
@@ Coverage Diff @@
## develop #60 +/- ##
===========================================
- Coverage 90.26% 90.23% -0.04%
===========================================
Files 32 32
Lines 2414 2426 +12
===========================================
+ Hits 2179 2189 +10
- Misses 235 237 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
JPchico
left a comment
JPchico
left a comment
There was a problem hiding this comment.
I think it looks good, just remove the print statement in the parser when dealing with the global variables.
any idea why the previous tests did not catch this issue?
sphuber
force-pushed
the
feature/parser-convert-total-runtime-seconds
branch
from
17e31ec to
c638370
Compare
Done.
Well, as far as I can tell, this parser was not tested whatsoever. I think that this parser was maybe the original one (together with We should maybe think how to harmonize all of these, because now there seems to be some duplication. |
Well that explains it. It is very nice that you added a test for the parser. I think that the plan was to do that sometime ago, but well clearly I must have forgotten to do it.
I agree 100% I think that the idea that @chrisjsewell had was that we should leave the old functions in case somebody needs them. But I think it might actually be best to remove them, or mark them for deprecation and make the test more modular. Since we end up having a lot of duplication and it is easy to miss something. So I'm all for removing the old parsers and calcjobs and keep the new ones and just run with that. |
That'd be fine with me as well, as long as I can still port over the functionality that I still added that allow adding a complete script as input and have the wall time seconds parsed. But if you are ok with that as well, then I am fine with opening a PR for that. |
|
@JPchico did anything still needed to be done for this PR? |
The `BaseLammpsCalculation` plugin was recently updated to allow specifying the exact script to run through the `script` input node. If the default parser is used in this case, which is the `LAMMPSBaseParser`, a non-zero exit code would always be returned because some of the output files that the parser wouldn't be there. These output files are normally written by the script that the plugin itself would create, but if a complete script is taken from inputs, it cannot be guaranteed that the script will produce these same outputs. The parser is updated to only return an exit code if an expected output file is not in the retrieved files if the `script` input was not defined. In addition, the `total_wall_time` key, if parsed by the log file parser, is parsed and converted to seconds and added as the `total_wall_time_seconds` key.
sphuber
force-pushed
the
feature/parser-convert-total-runtime-seconds
branch
from
c638370 to
a8590b5
Compare
2 participants
The
BaseLammpsCalculationplugin was recently updated to allow specifying the exact script to run through thescriptinput node. If the default parser is used in this case, which is theLAMMPSBaseParser, a non-zero exit code would always be returned because some of the output files that the parser wouldn't be there. These output files are normally written by the script that the plugin itself would create, but if a complete script is taken from inputs, it cannot be guaranteed that the script will produce these same outputs.The parser is updated to only return an exit code if an expected output file is not in the retrieved files if the
scriptinput was not defined. In addition, thetotal_wall_timekey, if parsed by the log file parser, is parsed and converted to seconds and added as thetotal_wall_time_secondskey.