Describe query parameters in OpenAPI schema

[CasperWA]

Closes #164
Fixes #165
Fixes #55

Remove PEP8 E251/E252 (unexpected spaces around keyword / parameter equals) from lint warnings.

Update page_page to page_number according to Materials-Consortia/OPTIMADE#187.

Due to pydantic: https://pydantic-docs.helpmanual.io/usage/schema/#unenforced-field-constraints the ge parameter cannot be used together with a non-standard type, e.g. NonnegativeInt. Instead the OpenAPI parameter is set directly, here minimum.

Regular expressions are added for response_fields and sort based on the regular expressions provided for an identifier in the OPTiMaDe spec.

This PR supersedes #56 by using Tuples in Lists for the lists in list values (cartesian_site_positions and lattice_vectors). Furthermore, an IntEnum is used to constrain dimension_types to only use 0 and 1s. The custom conlist has now been completely removed.
Most importantly, the constraints are reflected in the OpenAPI schema.

[CasperWA]

Maybe we should remove NonnegativeInt and use the qe parameters instead for Field, Query, etc.?

[ml-evs]

This looks great @CasperWA, if you write the constraints I'm happy to write the tests so it can be thoroughly checked!

[CasperWA]

Maybe we should remove NonnegativeInt and use the qe parameters instead for Field, Query, etc.?

I have now removed NonnegativeInt.

[CasperWA]

The latest commit upgrades dimension_types, moving it away from conlist and instead using the type and pydantic's Field to validate the property. This also means it is now represented correctly in the OpenAPI schema.

Concerning the two other uses of conlist (lattice_vectors and cartesian_site_positions) this may be more difficult, since it is list of lists, but it may be possible with the use of Tuple validation.

[shyamd]

Can we switch the enum names to PERIODIC and APERIODIC or something that is more descriptive? This provides a way of documenting the value in the code itself.

[CasperWA]

Can we switch the enum names to PERIODIC and APERIODIC or something that is more descriptive? This provides a way of documenting the value in the code itself.

Yup, was the next thing I was going to do. I like those names much better - this was merely to check if it worked to begin with.

It seems I have gotten rid of conlist completely now as well 😃

[shyamd]

Question: why are the components of lattice vectors are optional None?
Why not just:

Vector3D = Tuple[float,float,float]

rather than what is effectively:

Vector3D = Tuple[Optional[float],Optional[float],Optional[float]]

The final recommendation if anyone wants to do this is to validate fractional coordinates with something like this:

import numpy as np
from pydantic import BaseModel, validator

class FractionalVector3D(BaseModel):
    __root__ : Tuple[float,float,float]

    @validator("__root__")
    def check_fractional(cls, vector):
        vector = np.mod(vector,1.0).tolist()
        return vector

[CasperWA]

Question: why are the components of lattice vectors are optional None?

It is in the spec, see here.
However, it does state that all three elements of a vector MAY be null, not that a single element in a vector. So maybe we should check that?

Why not just:

Vector3D = Tuple[float,float,float]

rather than what is effectively:

Vector3D = Tuple[Optional[float],Optional[float],Optional[float]]

I deliberately didn't use Optional ... well, I originally did, but then chose to use Union with None instead to be more clear in the code; it's not optional, but it MAY be None ... I don't know if that makes sense? The result is the same in either case.

The final recommendation if anyone wants to do this is to validate fractional coordinates with something like this:

class FractionalVector3D(BaseModel):
    __root__ : Tuple[float,float,float]

    @validator("__root__")
    def check_fractional(cls, vector):
        vector = np.mod(vector,1.0).tolist()
        return vector

Nice. Although we were trying not to get numpy as a dependency.

[ml-evs]

We also don't have fractional coordinates in yet, see the discussion in Materials-Consortia/OPTiMaDe/#206.

[CasperWA]

We also don't have fractional coordinates in yet, see the discussion in Materials-Consortia/OPTiMaDe/#206.

True, but that PR still has some way to go I think.
In any case, let's maybe save these comments in an issue so we don't forget?

[shyamd]

I can recreate the fractional class pretty easily and you can search in github PRs so saving it shouldn't be a problem.

[CasperWA]

I can recreate the fractional class pretty easily and you can search in github PRs so saving it shouldn't be a problem.

Sure, it was also to have a place to discuss it and its implementation after this PR has been closed. But fine.
How about the state of this PR at the moment? Should we validate that a lattice vector may only be all None or all floats?
I guess there are still some tests missing for the various constraints - old and new?

[shyamd]

I mostly recommend the Tuple over the List whenever you know the exact size of the data because it shows up in the JSON schema as a little clearer. For instance, when I use Tuple[Vector3D, Vector3D, Vector3D] i get an JSON schema that looks like this:

Vector3D = Tuple[Union[float,None], Union[float,None], Union[float,None]]

class Matrix(BaseModel):
    __root__: Tuple[Vector3D,Vector3D,Vector3D]

print(Matrix.schema())

{'title': 'Matrix',
 'type': 'array',
 'items': [{'type': 'array',
   'items': [{'type': 'number'}, {'type': 'number'}, {'type': 'number'}]},
  {'type': 'array',
   'items': [{'type': 'number'}, {'type': 'number'}, {'type': 'number'}]},
  {'type': 'array',
   'items': [{'type': 'number'}, {'type': 'number'}, {'type': 'number'}]}]}

This is more verbose but it shows appropriately as a 3x3 array as opposed to an array constrained to size 3 of array of 3 floats.

[CasperWA]

I mostly recommend the Tuple over the List whenever you know the exact size of the data because it shows up in the JSON schema as a little clearer. (...)

Fair enough. I think it becomes quite verbose, but it may be clearer on the whole - especially when compared with cartesian_site_positions, where a List is indeed appropriate.

[shyamd]

This looks good for what this PR is supposed to do. We can discuss the float/None validation elsewhere before making a PR for that since it doesn't affect the OpenAPI schema.