Add model validators and regexp for chemical formulae fields

[ml-evs]

Closes #546 by adding validators and regexps to the chemical_formula_* OPTIMADE fields.

• Adds a test harness for modifying a single 'good' structure, to more easily test validator features beyond the bulk good/bad tests
• Does not ensure consistency between various formulae and species fields, which maybe it should... raises the question of how heavyweight our validators should be, and whether pydantic validation can/should be disabled by an implementation?

[ml-evs]

Have just discovered that all of our test data has the wrong element ordering for chemical_formula_reduced! Will fix this evening.

[codecov]

Codecov Report

Merging #547 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
+ Coverage   91.67%   91.77%   +0.10%     
==========================================
  Files          62       62              
  Lines        3182     3221      +39     
==========================================
+ Hits         2917     2956      +39     
  Misses        265      265              

Flag	Coverage Δ
#project	91.77% <100.00%> (+0.10%)	⬆️
#validator	64.63% <75.00%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/models/structures.py	95.79% <100.00%> (+0.65%)	⬆️
optimade/models/utils.py	91.56% <100.00%> (+1.15%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f34399...0ed2f49. Read the comment docs.

[ml-evs]

I think that's caught them all now...

[ml-evs]

Penny for anyone's thoughts? @CasperWA @shyamd

[CasperWA]

Great work @ml-evs !
This is some of the more intricate stuff that hasn't been fully specified in the specification, only in a human-readable way, so we might want to contribute some of this back to the specification. I'm thinking specifically of the regex.

I've put in some requested changes, suggested changes, observations, and other comments :)

[ml-evs]

From a resolved suggestion above:

Since fields are not cross-validated, I've added an extra test case with both C and H in them as "deformities". Do you have any opinions on my second point above? Should we add pairwise validators for all correlated fields?

[ml-evs]

I think that's dealt with everything, except my question above (which is for another PR).

Are we happy to add the regexp to our OpenAPI specification for now, but potentially demote it down to a validator property pending the meeting next week?

[CasperWA]

Only minor changes. Thanks @ml-evs.

[CasperWA]

Also, the changes in this PR introduces a lot of ValueError raises that are not tested. Do you think it's reasonable to add tests for these or simply leave it be? In an ideal world it would be nice to know that the raises are valid exits for the validators and it's not a code block that will never actually be reached.

[ml-evs]

Also, the changes in this PR introduces a lot of ValueError raises that are not tested. Do you think it's reasonable to add tests for these or simply leave it be? In an ideal world it would be nice to know that the raises are valid exits for the validators and it's not a code block that will never actually be reached.

ValueErrors get turned into the pydantic ValidationErrors that we are testing for with all our entry tests right? At the very least we have a match on the message that it returns an error corresponding to the field we think is incorrect. Everything I've added here should be tested in the new tests that use the good_structure fixture, plus a lot of things we weren't testing directly before

[CasperWA]

Also, the changes in this PR introduces a lot of ValueError raises that are not tested. Do you think it's reasonable to add tests for these or simply leave it be? In an ideal world it would be nice to know that the raises are valid exits for the validators and it's not a code block that will never actually be reached.

ValueErrors get turned into the pydantic ValidationErrors that we are testing for with all our entry tests right? At the very least we have a match on the message that it returns an error corresponding to the field we think is incorrect. Everything I've added here should be tested in the new tests that use the good_structure fixture, plus a lot of things we weren't testing directly before

Right. It just seems 6 new lines are missed for the Structure model validators, possibly not being tested? See here.

[ml-evs]

Right. It just seems 6 new lines are missed for the Structure model validators, possibly not being tested? See here.

Ah, those are the checks that aren't triggered as they are already caught by the overall field regexp. How much do we want to placate the coverage gods?

[CasperWA]

Right. It just seems 6 new lines are missed for the Structure model validators, possibly not being tested? See here.

Ah, those are the checks that aren't triggered as they are already caught by the overall field regexp. How much do we want to placate the coverage gods?

So what you're saying is that they are now never reached?... :)

[ml-evs]

Right. It just seems 6 new lines are missed for the Structure model validators, possibly not being tested? See here.

Ah, those are the checks that aren't triggered as they are already caught by the overall field regexp. How much do we want to placate the coverage gods?

So what you're saying is that they are now never reached?... :)

Well, the checks themselves are, if they raise an error then we can diagnose any future problems that we have introduced into the regexp. Equally, if we decide tomorrow that we shouldn't include the regexp in the schema, then we'll have to rely on these checks again (or at least stop fastapi from putting the regexp in the schema itself)

[CasperWA]

So what you're saying is that they are now never reached?... :)

Well, the checks themselves are, if they raise an error then we can diagnose any future problems that we have introduced into the regexp. Equally, if we decide tomorrow that we shouldn't include the regexp in the schema, then we'll have to rely on these checks again (or at least stop fastapi from putting the regexp in the schema itself)

Personally, I'd prefer the cleaner option; to cut away the fat/non-used code.
This will diminish confusion and if we wish to revert, we can always do this from the git history.

[ml-evs]

So what you're saying is that they are now never reached?... :)

Well, the checks themselves are, if they raise an error then we can diagnose any future problems that we have introduced into the regexp. Equally, if we decide tomorrow that we shouldn't include the regexp in the schema, then we'll have to rely on these checks again (or at least stop fastapi from putting the regexp in the schema itself)

Personally, I'd prefer the cleaner option; to cut away the fat/non-used code.
This will diminish confusion and if we wish to revert, we can always do this from the git history.

Understood; let's park this until the meeting tomorrow, in case they don't like the regexp.

• If we can adopt the regexp in the spec, then the extra checks can be removed.
• Alternatively, w could run the specific field validators with pre=True or always=True so they are triggered even when the regexp doesn't match, to provide a more informative error

[ml-evs]

In the OPTIMADE meeting today it was agreed that putting the chemical formula regexp into the schema is good, so we can proceed here and remove the extra validators that never fail. The discussion also included the possibility of expanding the regexp to also check for element symbols.

[ml-evs]

In the OPTIMADE meeting today it was agreed that putting the chemical formula regexp into the schema is good, so we can proceed here and remove the extra validators that never fail. The discussion also included the possibility of expanding the regexp to also check for element symbols.

Regexp has been left in, any validators added by this PR that were not being hit were either removed or tests were added to hit them. Should be good to go.

[CasperWA]

All good on my part.
I still did a few suggestions, but they are not worth not approving for :)