Add error handlers to check for chgcar, aeccar integrity

[mkhorton]

See atomate PR hackingmaterials/atomate#276

Needs further investigation; @jmmshn could you attach some sample bad outputs? Suspect it may be an mpi-related issue where lines are getting transposed.

[jmmshn]

I will add some kind of simple smoothness check for the CHGCAR and AECCAR.
I have only seen the AECCAR exhibit this problem (picture below)

[mkhorton]

What iso-surface level is that screenshot taken at? Almost looks like the grid points are off by one... Is there any correlation between grid dimension and this issue occurring, or is it random?

[shyuep]

Just to be clear - the role of custodian is not to catch things which cannot be fixed. I am not sure what is causing this smoothness issue, but unless there is a recipe to be taken, this is not something for cusotdian.

[jmmshn]

I haven't collected statistics on the success rates but rerunning the calculations seemed to fix some of these.

[mkhorton]

@shyuep Surely it needs to trigger a NonRecoverableError? In my mind it would be bad for Custodian to raise no errors for a calculation that has produced bad output.

[shyuep]

@mkhorton It depends on the definition of "bad output". CHGCARs are somewhat optional in a lot of the analysis that we do. E.g., if you are just relaxing a structure or just want an energy. If this is a desired check, I would certainly put it as a ChgcarValidator - not a Handler, which implies real-time checking. And this validator would be one where only people who care about CHGCARs would bother using.

[mkhorton]

Ok, that sounds fair.

[jmmshn]

OK sound good I will add an example "bad" output and the validator.
I would still like to keep the checks on the atomate side as a fail-safe.

[shyuep]

@jmmshn @computron It is up to you since I don't want to impose my will on atomate, but my general advice is that software checks should be performed in ONE place. I generally do not like duplicating checks within or across codes. That duplication merely serves to create headaches - some new student fiddles with the atomate checks and forgets to make the same changes to the custodian validators. It is fine if you want atomate to check a different set of things that are complementary to custodian checks. But the same check should not be in two places.

As a general principle, MP-related stuff should be on the basis on "one source of truth". For input sets, that should be pymatgen. For run-time error handling, that should be custodian. For workflows, that would be atomate.

[mkhorton]

One solution here might be to write a ChgcarValidator in custodian, but to explicitly import that and use it in the atomate drone to validate when chgcar ingestion is attempted. That way code implementation is not duplicated.

[shyuep]

That is fine. Though I would have thought the ChgcarValidator would simply be passed into the custodian call in atomate?

[jmmshn]

I think the concern was about ingestion of old tasks into MP releases. For that, I thank what Matt suggested makes sense.

I have a simple test in the atomate PR that just checks for positive definiteness of the AECCAR which I will replace with a more sophisticated check after I update custodian.