aiida-vasp / parsevasp

A general parser for VASP
MIT License
13 stars 13 forks source link

Added a check for VASP start and fixed quote consistency #122

Closed espenfl closed 1 year ago

espenfl commented 1 year ago

Here we add a trigger for the stream parser that ends up setting an INFO message if VASP was started. However, note that this check tells nothing about the run being successful, meaning complete or not. Only that VASP has been started.

espenfl commented 1 year ago

@JPchico @atztogo @zhubonan Would be sweet if one of you could have a quick look at this. It preps to use this in AiiDA-VASP to set the right exit code when vasp_output is not empty (for instance if mpirun etc. is not available). Only states that VASP was started, not that it executed cleanly.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 93.75% and project coverage change: +0.24 :tada:

Comparison is base (fa6465b) 80.36% compared to head (d6f9922) 80.60%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #122 +/- ## =========================================== + Coverage 80.36% 80.60% +0.24% =========================================== Files 11 13 +2 Lines 3334 3478 +144 =========================================== + Hits 2679 2803 +124 - Misses 655 675 +20 ``` | [Impacted Files](https://codecov.io/gh/aiida-vasp/parsevasp/pull/122?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp) | Coverage Δ | | |---|---|---| | [parsevasp/stream.py](https://codecov.io/gh/aiida-vasp/parsevasp/pull/122?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp#diff-cGFyc2V2YXNwL3N0cmVhbS5weQ==) | `90.48% <92.00%> (+1.84%)` | :arrow_up: | | [parsevasp/vasprun.py](https://codecov.io/gh/aiida-vasp/parsevasp/pull/122?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp#diff-cGFyc2V2YXNwL3Zhc3BydW4ucHk=) | `83.69% <100.00%> (ø)` | | ... and [2 files with indirect coverage changes](https://codecov.io/gh/aiida-vasp/parsevasp/pull/122/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiida-vasp)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

atztogo commented 1 year ago

@espenfl, I don't see what this PR means. How does this couple with external code like aiida?

zhubonan commented 1 year ago

Looks fine to me! One thing that I am not sure is that the notification is emitted when there is no problem, but in other cases the notifications are there for errors.

This logic is fine, but there is a potential problem when working with aiida-vasp - one must make sure parservasp is up-to-date otherwise, aiida-vasp (with the checks for the new INFO notification) will think every calculation is not run. This might comes as a surprise if one updates aiida-vasp but did not do so for parsevasp.

Maybe a better solution is to have a negative match where the notification is emitted when the stream does not detect vasp. regex?

The bits we need to change aiida-vasp is here: https://github.com/aiida-vasp/aiida-vasp/blob/a0fc60b15be0f3e2c286659b2252ff57e416fa2f/aiida_vasp/parsers/vasp.py#L277-L317

espenfl commented 1 year ago

@espenfl, I don't see what this PR means. How does this couple with external code like aiida?

This is to give the option in aiida-vasp to fix the fact that we set an exit code that is not so useful at first for the user in case VASP is never started. There, we now only check if the vasp_output is present, which is not enough.

atztogo commented 1 year ago

@espenfl, I don't see what this PR means. How does this couple with external code like aiida?

This is to give the option in aiida-vasp to fix the fact that we set an exit code that is not so useful at first for the user in case VASP is never started. There, we now only check if the vasp_output is present, which is not enough.

I see. Thanks for your explanation. By the way, this commit includes refactoring (which could be another PR, too). How did it happen?

espenfl commented 1 year ago

I see. Thanks for your explanation. By the way, this commit includes refactoring (which could be another PR, too). How did it happen?

I think the only thing in here is a few linting changes in the yaml file and some others that was due to version updates to make the tests pass. We should fix the test and pre-commit to aiida-vasp[pre-commit] etc. I think, but that would be a separate PR. Also, that would introduce a dependency. Willing to accept that to reduce maintenance. Then we can run a nightly to check if something needs updates.

espenfl commented 1 year ago

Okey, I have now updated this PR. Also fixed a pop during iteration of the triggers. We can now defined the tag inverse on the triggers in the config, which has the following behavior:

Typical use case is as @zhubonan mentions above, where we want to detect something in the stream, but only set something when we do not detect it, e.g. introduced here: if VASP was executed (looks for parts of the vasp.x.y.z etc. string which seems to always be present).

espenfl commented 1 year ago

@zhubonan Do you mind having another look. Thanks.