Should available quantities and entries in parser be None if not available

[espenfl]

Should we make all quantities None if they are not parsable. This also includes sub-elements in dicts. I would at least like this to be consistent and it would be hard to do for something else than None. Using None can cause issues and a bit more boilerplate than necessary, but then we at least know that if we detect None it is not parsed. @atztogo @zhubonan @JPchico what do you think?

[atztogo]

@espenfl , What do you mean "all quantities"? User's 'incar' input?

[zhubonan]

The question is what does not parsable mean?
I think we should distinguish between the properties is simply not present (say I used ISIF=1 but the parser wants stress) or it is because that the parser encountered an error during the parsing.

[espenfl]

@espenfl , What do you mean "all quantities"? User's 'incar' input?

I mean every single quantity we return. That could be the main quantity that is requested, e.g. PARSABLE_QUANTITIES or any sub entry in that, say if we have a dict.

[espenfl]

The question is what does not parsable mean? I think we should distinguish between the properties is simply not present (say I used ISIF=1 but the parser wants stress) or it is because that the parser encountered an error during the parsing.

Yes, and since we can try different parsers for the same quantity an exception should only be raised when we have exhausted all options. And for quantities that then is not being able to be parser by a specific content parser, should we return None for that? This should then also include sub entries I think as sometimes we are interested in that and if we do not also make sure that is None and check it, the framework would believe parsing was fine. This check would be done pre or in the NodeComposer. E.g. say we need the magnetization in one direction for ion 7 for a particular node in some future workchain (artificial construct as magnetization is now only present in OUTCAR, but imagine it would be available in multiple parsers). If one parser failed to set the decomposed, but another not, we would have to know that pre or in the NodeComposer to be able to continue the search, even though the magnetization quantity was not None. I think we mostly do this, but noticed a few things that was left as empty lists etc. instead of None.

[atztogo]

"quantity" is used in different meaning in different context. This is what I needed to struggle most in my previous refactoring of parser. I wrote memo about it here, https://github.com/aiida-vasp/aiida-vasp/wiki/Memo-on-parser#variable-names. I didn't change the original variable names but probably now it is better to define obvious names.

If I remember correctly, currently parser is made of a series of screenings, plus NodeComposer that is not a screening. So if my understanding is correct, what I want to be sure is (1) which step of screenings (or NodeComposer) we are discussing, and (2) which intermediate "quantity" we are discussing. So before discussing further, it is probably important to define the workflow and intermediate values (and terminology) in a documentation.

[espenfl]

"quantity" is used in different meaning in different context. This is what I needed to struggle most in my previous refactoring of parser. I wrote memo about it here, https://github.com/aiida-vasp/aiida-vasp/wiki/Memo-on-parser#variable-names. I didn't change the original variable names but probably now it is better to define obvious names.

Yes, we should at least review and make sure to do any changes that makes logical sense.

If I remember correctly, currently parser is made of a series of screenings, plus NodeComposer that is not a screening. So if my understanding is correct, what I want to be sure is (1) which step of screenings (or NodeComposer) we are discussing, and (2) which intermediate "quantity" we are discussing. So before discussing further, it is probably important to define the workflow and intermediate values (and terminology) in a documentation.

We are not yet discussing the screening or NodeComposer. Only what the content_parsers should return if it is not able to parse. This also goes for sub entries in dicts. I think it would be nice to return a consistent value across the board if it can not be parsed. We should also raise some exceptions, at least for basic failures which we can use to act later. During testing when rewriting the content_parsing I noticed a few entries were empty lists instead of None for instance. My suggestion would be that we try to set None for anything that does not contain another non-empty Python data type.

[atztogo]

Only what the content_parsers should return if it is not able to parse.

I could not understand your question until here.