Fix field optonality inconsistency in schema

[ml-evs]

As it stands, all SHOULD fields are non-optional in our pydantic models. Somehow this did not apply to nperiodic_dimensions and lattice_vectors, so this PR fixes that.

There is still an outstanding question about how we should handle these fields in pydantic and in OpenAPI. Are "SHOULD" fields "required"? It is impossible to validate our StructureResource automatically with pydantic with some fields missing, unless we made them Optional and added conditionals to the pydantic validators.

We might be able to get around this in the implementation validator by just ignoring documents that are not "full" of all the "SHOULD" fields (i.e. not returning an error), but then obviously their contents will not be well-checked.

I've written this description in this PR for posterity and will probably cross-post it to the appropriate issue.

[codecov]

Codecov Report

Merging #482 into master will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
- Coverage   91.67%   91.64%   -0.04%     
==========================================
  Files          60       60              
  Lines        2848     2848              
==========================================
- Hits         2611     2610       -1     
- Misses        237      238       +1     

Flag	Coverage Δ
#unittests	91.64% <100.00%> (-0.04%)	⬇️

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

Impacted Files	Coverage Δ
optimade/models/structures.py	95.08% <100.00%> (-0.55%)	⬇️

---

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 3d4b5bc...8cec2c3. Read the comment docs.

[CasperWA]

As it stands, all SHOULD fields are non-optional in pydantic. Somehow this did not apply to nperiodic_dimensions and lattice_vectors, so this PR fixes that.

When you say pydantic, you mean our pydantic models, right? 😃

There is still an outstanding question about how we should handle these fields in pydantic and in OpenAPI. Are "SHOULD" fields "required"? It is impossible to validate our StructureResource automatically with pydantic with some fields missing, unless we made them Optional and added conditionals to the pydantic validators.

We might be able to get around this in the implementation validator by just ignoring documents that are not "full" of all the "SHOULD" fields (i.e. not returning an error), but then obviously their contents will not be well-checked.

In my opinion, the approach should be that a SHOULD field that is supplied carries with it certain requirements. What I mean is, the length of the cartesian_site_positions list MUST equal the length of species_at_sites, hence if cartesian_site_positions is supplied it requires species_at_sites to also be supplied, even though both are SHOULD fields.
In essence the pydantic model validations will then ensure this field dependency, and also be a valid reason for failing a given OPTIMADE structure if the field dependency requirement is not met - again, even though both fields are "only" on the SHOULD level.

We should then make sure our pydantic model validations indeed hold true for these inter-field dependencies, while still being fine if a SHOULD field that is otherwise not required by another field, is left out of the structure.

Edit: So all in all, I think we should make all SHOULD fields optional (but maybe not in this PR, but rather in #453). Fixing this inconsistency will make sure we hopefully get all fields into the right logical state by the end of #453.

[ml-evs]

When you say pydantic, you mean our pydantic models, right? smiley

Pretend it always said that :)

We should then make sure our pydantic model validations indeed hold true for these inter-field dependencies, while still being fine if a SHOULD field that is otherwise not required by another field, is left out of the structure.

I think this is our only option (no matter what we do with the schema) which will be a whole lot of fun to write and test... see my comments in the linked issue.

Either way, this should be merged for now, and I think we should include it in the upstream PR so we are at least self-consistent at this stage.

[CasperWA]

Thanks @ml-evs