search

aiidateam / aiida-quantumespresso

The official AiiDA plugin for Quantum ESPRESSO
https://aiida-quantumespresso.readthedocs.io
Other
52 stars 77 forks source link

``PwCalculation``: add exit code and handler for ``scale_h`` error #913

Closed bastonero closed 1 year ago

bastonero commented 1 year ago

This PR adds the exit code and handler for the scale_h error presented in #752.

bastonero commented 1 year ago

We might think of changing the name of the error to something more sensible if necessary. Also, it would be great to verify that the tests I added satisfy what actually @mbercx experienced. Currently, I copy/pasted the tests of vcrelax_failed_nstep, as it contained the complete xml file. AFAIU, when this error appears, we are expecting an xml which can be parsed and contains an output structure that can be used for restarting.

sphuber commented 1 year ago

Also, another thing I just realized. With your recent changes to parsing the CRASH file and each error test having two versions (one with CRASH file, and one with stdout) the reference files are being duplicated, adding a lot of extra weight to the repo. I wonder if we can simply use relative symlinks to avoid the content duplication. Could you try to replace the stdout copy of the stdout and XML files with symlinks to the defualt one?

bastonero commented 1 year ago

Also, another thing I just realized. With your recent changes to parsing the CRASH file and each error test having two versions (one with CRASH file, and one with stdout) the reference files are being duplicated, adding a lot of extra weight to the repo. I wonder if we can simply use relative symlinks to avoid the content duplication. Could you try to replace the stdout copy of the stdout and XML files with symlinks to the defualt one?

Sure. Nevertheless, I think we can do it only for XML, as the stdout will differ depending whether the error output is in the stdout or in CRASH. Therefore I guess having duplicated stdout is unavoidable. What do you think?

sphuber commented 1 year ago

stdout will differ depending whether the error output is in the stdout or in CRASH.

Is this because in the newer versions of QE, the error information is only in CRASH and not the stdout? At least before, it used to be in both, correct? But if so, and the stdout no longer contains the error info in latest QE versions, then yes, you will have to duplicate, but that is not too bad since anyway you probably just want to include the minimal output in the stdout since we are just testing the error is parsed correctly. Don't really care that all other parameters are correctly parsed.

bastonero commented 1 year ago

Is this because in the newer versions of QE, the error information is only in CRASH and not the stdout?

Exactly.

At least before, it used to be in both, correct?

Right, in fact we should maybe put the CRASH there as well, to check nothing crashes for some weird reason. And this we can link.

But if so, and the stdout no longer contains the error info in latest QE versions, then yes, you will have to duplicate, but that is not too bad since anyway you probably just want to include the minimal output in the stdout since we are just testing the error is parsed correctly. Don't really care that all other parameters are correctly parsed.

Absolutely!

sphuber commented 1 year ago

Thanks @bastonero