Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update model descriptions and openapi.json for 1.0.0-rc2 #351

Merged
merged 8 commits into from Jun 25, 2020

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jun 16, 2020

This PR updates the field descriptions and OpenAPI schema, bringing them up to date with 1.0.0-rc2 (closes #332) and turning them into markdown for better docs (closes #307, closes #184).

Unfortunately this process is still very manual. You can get the spec and convert it into the appropriate markdown format with the task invoke get-markdown-spec, provided you have pandoc installed on your machine. There's then a lot of tedious copy and pasting...

This PR is based off #350, which will need to be merged first.

Still to-do:

  • base entries
  • structures
  • references
  • index meta-db
  • links
  • JSON API/OPTIMADE JSON API models
  • Base info
  • query parameters

@CasperWA CasperWA added the blocked For issues/PRs that are blocked by required changes/clarifications to the specification. label Jun 16, 2020
@ml-evs ml-evs force-pushed the ml-evs/openapi_updates branch 3 times, most recently from 04219b3 to 1533ad0 Compare June 22, 2020 17:56
@ml-evs ml-evs marked this pull request as ready for review June 22, 2020 17:56
@ml-evs ml-evs added OpenAPI OpenAPI / Swagger related issue priority/high Issue or PR with a consensus of high priority schema Concerns the schema models and removed blocked For issues/PRs that are blocked by required changes/clarifications to the specification. labels Jun 22, 2020
@ml-evs ml-evs changed the title [WIP] Update model descriptions and openapi.json for 1.0.0-rc2 Update model descriptions and openapi.json for 1.0.0-rc2 Jun 22, 2020
@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #351 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #351   +/-   ##
=======================================
  Coverage   90.92%   90.92%           
=======================================
  Files          54       54           
  Lines        2336     2336           
=======================================
  Hits         2124     2124           
  Misses        212      212           
Flag Coverage Δ
#unittests 90.92% <100.00%> (ø)
Impacted Files Coverage Δ
optimade/models/baseinfo.py 94.73% <ø> (ø)
optimade/models/index_metadb.py 100.00% <ø> (ø)
optimade/models/links.py 100.00% <ø> (ø)
optimade/models/optimade_json.py 93.82% <ø> (ø)
optimade/models/references.py 97.61% <ø> (ø)
optimade/models/structures.py 95.18% <ø> (ø)
optimade/server/query_params.py 100.00% <ø> (ø)
optimade/models/entries.py 100.00% <100.00%> (ø)
optimade/models/jsonapi.py 90.52% <100.00%> (ø)

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 fe45f97...a99d3de. Read the comment docs.

@ml-evs ml-evs self-assigned this Jun 23, 2020
@ml-evs ml-evs requested review from CasperWA, shyamd and fekad and removed request for CasperWA June 23, 2020 12:29
@fekad
Copy link
Contributor

fekad commented Jun 23, 2020

You can clean up the code a little by using indentations with the python's built-in cleandoc function. I mean it is completely fine in the current state and this is just a matter of taste so please feel free to ignore my comments about it.
Nevertheless you can find a small code snippet below:

from pydantic import BaseModel, Field
from typing import List
from inspect import cleandoc


class Species(BaseModel):
    name: str = Field(
        ...,
        decsription="""Gives the name of the species; the **name** value MUST be unique in the `species` list.""",
    )

    chemical_symbols: List[str] = Field(
        ...,
        description="""MUST be a list of strings of all chemical elements composing this species. Each item of the list MUST be one of the following:
- a valid chemical-element name, or
- the special value `"X"` to represent a non-chemical element, or
- the special value `"vacancy"` to represent that this site has a non-zero probability of having a vacancy (the respective probability is indicated in the `concentration` list, see below).
If any one entry in the `species` list has a `chemical_symbols` list that is longer than 1 element, the correct flag MUST be set in the list `structure_features` (see property [structure_features](#structure_features)).""",
    )

    concentration: List[float] = Field(
        ...,
        description="""MUST be a list of floats, with same length as `chemical_symbols`. The numbers represent the relative concentration of the corresponding chemical symbol in this species. The numbers SHOULD sum to one. Cases in which the numbers do not sum to one typically fall only in the following two categories:
- Numerical errors when representing float numbers in fixed precision, e.g. for two chemical symbols with concentrations `1/3` and `2/3`, the concentration might look something like `[0.33333333333, 0.66666666666]`. If the client is aware that the sum is not one because of numerical precision, it can renormalize the values so that the sum is exactly one.
- Experimental errors in the data present in the database. In this case, it is the responsibility of the client to decide how to process the data.
Note that concentrations are uncorrelated between different site (even of the same species).""",
    )


class Species_new(BaseModel):
    name: str = Field(
        ...,
        decsription="""Gives the name of the species; the **name** value MUST be unique in the `species` list.""",
    )

    chemical_symbols: List[str] = Field(
        ...,
        description=cleandoc("""MUST be a list of strings of all chemical elements composing this species. Each item of the list MUST be one of the following:
            - a valid chemical-element name, or
            - the special value `"X"` to represent a non-chemical element, or
            - the special value `"vacancy"` to represent that this site has a non-zero probability of having a vacancy (the respective probability is indicated in the `concentration` list, see below).
            If any one entry in the `species` list has a `chemical_symbols` list that is longer than 1 element, the correct flag MUST be set in the list `structure_features` (see property [structure_features](#structure_features))."""),
    )

    concentration: List[float] = Field(
        ...,
        description=cleandoc("""MUST be a list of floats, with same length as `chemical_symbols`. The numbers represent the relative concentration of the corresponding chemical symbol in this species. The numbers SHOULD sum to one. Cases in which the numbers do not sum to one typically fall only in the following two categories:
            - Numerical errors when representing float numbers in fixed precision, e.g. for two chemical symbols with concentrations `1/3` and `2/3`, the concentration might look something like `[0.33333333333, 0.66666666666]`. If the client is aware that the sum is not one because of numerical precision, it can renormalize the values so that the sum is exactly one.
            - Experimental errors in the data present in the database. In this case, it is the responsibility of the client to decide how to process the data.
            Note that concentrations are uncorrelated between different site (even of the same species)."""),
    )

    class Config:
        title = 'Species'


if __name__ == '__main__':
    print(Species.schema_json(indent=2))
    print(Species_new.schema_json(indent=2))
    print(Species.schema_json(indent=2) == Species_new.schema_json(indent=2))

@ml-evs
Copy link
Member Author

ml-evs commented Jun 24, 2020

You can clean up the code a little by using indentations with the python's built-in cleandoc function. I mean it is completely fine in the current state and this is just a matter of taste so please feel free to ignore my comments about it.

This could be useful, but don't think we should do it right now as this PR is blocking rc2 of the spec.

Can anyone review this today so we can make the PR to update the OpenAPI schemas in the spec repo?

optimade/models/jsonapi.py Outdated Show resolved Hide resolved
optimade/models/structures.py Outdated Show resolved Hide resolved
optimade/models/references.py Outdated Show resolved Hide resolved
optimade/models/jsonapi.py Outdated Show resolved Hide resolved
optimade/models/structures.py Outdated Show resolved Hide resolved
optimade/models/jsonapi.py Outdated Show resolved Hide resolved
optimade/models/optimade_json.py Outdated Show resolved Hide resolved
optimade/models/optimade_json.py Outdated Show resolved Hide resolved
optimade/models/optimade_json.py Outdated Show resolved Hide resolved
optimade/models/optimade_json.py Outdated Show resolved Hide resolved
@fekad fekad self-requested a review June 24, 2020 11:57
optimade/models/structures.py Outdated Show resolved Hide resolved
optimade/models/structures.py Outdated Show resolved Hide resolved
Co-authored-by: Adam Fekete <fekad@users.noreply.github.com>
fekad
fekad previously approved these changes Jun 24, 2020
@ml-evs
Copy link
Member Author

ml-evs commented Jun 24, 2020

Just been scrolling through the generated mkdocs locally, looks like it doesn't handle class docstrings in the same way as the descriptions (it treats docstring indents as code blocks) so I've pushed a change to them too.

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right! Thanks @ml-evs !

I have gone through all the files now, except structures.py.
I'll go through that momentarily, however, I see stuff is happening, so I'll just get these comments and suggestions out there for now.

Hidden in the comments are some general suggestions and comments as well, so please read carefully.

optimade/models/baseinfo.py Outdated Show resolved Hide resolved
optimade/models/entries.py Show resolved Hide resolved
optimade/models/entries.py Show resolved Hide resolved
optimade/models/entries.py Show resolved Hide resolved
optimade/models/entries.py Show resolved Hide resolved
optimade/models/optimade_json.py Show resolved Hide resolved
optimade/models/optimade_json.py Outdated Show resolved Hide resolved
optimade/models/optimade_json.py Outdated Show resolved Hide resolved
tasks.py Show resolved Hide resolved
optimade/models/references.py Outdated Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/openapi_updates branch 2 times, most recently from f5860a8 to 7f7c01a Compare June 24, 2020 13:52
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having a hard time processing all the changes in structures.py.
I simply trust you have done this properly 😅
In the sense that all other comments are taken into account from my other review and also pushed through for this section (those that are necessary), which I think would only be the concern of the indentation?

optimade/models/structures.py Show resolved Hide resolved
optimade/models/structures.py Outdated Show resolved Hide resolved
@ml-evs ml-evs force-pushed the ml-evs/openapi_updates branch 3 times, most recently from 2f4bb37 to 513e9b7 Compare June 24, 2020 15:15
@CasperWA
Copy link
Member

We should also check the query parameters.

Co-authored-by: Casper Welzel Andersen <CasperWA@users.noreply.github.com>
Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been through query params now as well.

optimade/server/query_params.py Outdated Show resolved Hide resolved
optimade/server/query_params.py Outdated Show resolved Hide resolved
optimade/server/query_params.py Outdated Show resolved Hide resolved
optimade/server/query_params.py Outdated Show resolved Hide resolved
optimade/server/query_params.py Outdated Show resolved Hide resolved
optimade/server/query_params.py Outdated Show resolved Hide resolved
optimade/server/query_params.py Outdated Show resolved Hide resolved
optimade/server/query_params.py Outdated Show resolved Hide resolved
Also, apply review comments.
@CasperWA
Copy link
Member

CasperWA commented Jun 25, 2020

Instead of returning to one-line descriptions (as I have just done), we could turn the strings into variables somewhere and simply reference these variables in the two query classes?

For now, this should be fine though, I think.

@ml-evs
Copy link
Member Author

ml-evs commented Jun 25, 2020

Instead of returning to one-line descriptions (as I have just done), we could turn the strings into variables somewhere and simply reference these variables in the two query classes?

For now, this should be fine though, I think.

Fine by me, have you implemented your requested formatting changes in your commit?

@CasperWA
Copy link
Member

Fine by me, have you implemented your requested formatting changes in your commit?

I have, as I also wrote in the commit message :)

@ml-evs
Copy link
Member Author

ml-evs commented Jun 25, 2020

Let's get this in then!

@CasperWA CasperWA merged commit 7471190 into master Jun 25, 2020
@CasperWA CasperWA deleted the ml-evs/openapi_updates branch June 25, 2020 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OpenAPI OpenAPI / Swagger related issue priority/high Issue or PR with a consensus of high priority schema Concerns the schema models
Projects
None yet
3 participants