Update models according to changes during CECAM 2020 meeting #350

ml-evs · 2020-06-15T09:39:39Z

Closes #343.

Added as an optional field, though we need a discussion about which fields are truly optional in the models (as almost everything is optional in the spec).

@CasperWA I can't work on this again until this evening, if you wanted to add other models to this PR then feel free to edit.

Closes #342
Closes #344
Closes #345

codecov · 2020-06-15T09:45:21Z

Codecov Report

Merging #350 into master will increase coverage by 0.32%.
The diff coverage is 98.31%.

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
+ Coverage   90.59%   90.92%   +0.32%     
==========================================
  Files          55       54       -1     
  Lines        2276     2336      +60     
==========================================
+ Hits         2062     2124      +62     
+ Misses        214      212       -2

Flag	Coverage Δ
#unittests	`90.92% <98.31%> (+0.32%)`	⬆️

Impacted Files	Coverage Δ
optimade/adapters/structures/pymatgen.py	`100.00% <ø> (ø)`
optimade/server/routers/info.py	`96.15% <ø> (ø)`
optimade/models/structures.py	`95.18% <96.72%> (+0.78%)`	⬆️
optimade/adapters/structures/aiida.py	`100.00% <100.00%> (ø)`
optimade/adapters/structures/ase.py	`100.00% <100.00%> (ø)`
optimade/adapters/structures/cif.py	`100.00% <100.00%> (ø)`
optimade/adapters/structures/jarvis.py	`100.00% <100.00%> (ø)`
optimade/adapters/structures/proteindatabank.py	`100.00% <100.00%> (ø)`
optimade/models/entries.py	`100.00% <100.00%> (ø)`
optimade/models/index_metadb.py	`100.00% <100.00%> (+5.55%)`	⬆️
... and 6 more

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 3c6c84d...5bce8e5. Read the comment docs.

CasperWA · 2020-06-15T09:48:58Z

Added as an optional field, though we need a discussion about which fields are truly optional in the models (as almost everything is optional in the spec).

Exactly this is related to the issue #198.

CasperWA · 2020-06-15T12:38:54Z

Changes to models:

nattached and attached OPTIONAL fields to species.
Removed possibility for having unknown positions (i.e., null values in a position.
Updated structure_features:
- Remove unknown_positions flag.
- Add site_attachments and implicit_atoms flags.
Add the OPTIONAL properties aggregate and no_aggregate_reason to LinksResource.
aggregate values MUST be in a specific (literal) set, ~~a validator has been added for this~~.
Add the OPTIONAL field type to properties under EntryInfoResource.
New Enum called DataType to list out the OPTIMADE Data types.
It has also gotten some useful methods:
- get_values(): Retrieve a sorted list of all values
- from_python_type(): Return enumeration based on a Python type (pass either a type, object or a string representation of the type).
- from_json_type(): Return enumeration based on a JSON type or OpenAPI-specific "format" of type.
  In the OpenAPI specification type is mandatory, hence all pydantic model schemas will always return a type. It may be further specified by an additional field format, which could hold values such as date-time, email, etc.
Use enumerations for various "literal" values (since Literal cannot be used):
- aggregate (in LinksResourceAttributes)
- link_type (in LinksResourceAttributes)
- key in index meta-database's ´/infoendpointrelationships(inIndexInfoResource`)
- structure_features (in StructureResourceAttributes)

Changes to code:

Removed padding of positions and testing unknown positions for StructureAdapter and related conversion functions.
Creating type field in entry listing info endpoints' properties dictionary based on the models' OpenAPI schema properties' format value, falling back on the mandatory type key.
Use new enumerations where necessary.

CasperWA · 2020-06-15T13:08:41Z

We are currently only testing the validators and validity of the structures models explicitly.
This should be extended to include references and links (at minimum).

ml-evs · 2020-06-15T18:33:54Z

optimade/models/optimade_json.py

+    @classmethod
+    def from_python_type(cls, python_type: Union[type, str, object]):
+        """Get OPTIMADE data type from a Python type"""
+        mapping = {


Could we define these mappings (incl. JSON map below) as properties?
e.g.

@property def python_to_optimade_type_map(...):

and

@property json_to_optimade_type_map(...)

Tried to do this, the issue is that then it's not reachable on the class level, i.e., DataType.from_json_type("number") doesn't work anymore.
If they're both instance methods/properties, then you first have to instantiate it, i.e., DataType("float").from_json_type("date-time"), which is quite confusing, since "float" is completely ignored in this process.

Do you have a smart way of doing this? Other than just making it a static method perhaps?

Yeah, I'm trying to do it with static methods now, but that of course doesn't work since we're utilizing the self referenced enumerations ...
Could I make the mappings enumerations? Or should they simply be outside the Enum?

Ah, hmm, didn't appreciate this when I left my suggestion. In that case, I would leave it as is for now; I like the classmethod access and this is a clean way of doing it.

I'll profile the manipulation of our models at some point so we can target our optimisations, then we can decide where to put the effort in.

optimade/server/routers/utils.py

ml-evs · 2020-06-15T18:42:22Z

We are currently only testing the validators and validity of the structures models explicitly.
This should be extended to include references and links (at minimum).

Agreed, one for #104, I guess...

CasperWA · 2020-06-15T22:55:14Z

I've taken care of the merge conflicts with the test structures and found that we weren't actually testing test_more_structures.json, so I updated this, testing it again, and found that some of the both new and old validators were wrong.

I had to change the initialization of species_at_sites and species in order to have species available in the validation of species_at_sites. species does not really care about species_at_sites, so it's fine.

We apparently validated that a species MUST be in species_at_sites as well, which is not a hard MUST. So I removed this validation. It seems we should really go through all of our validators again...

CasperWA · 2020-06-15T23:52:54Z

We need to make some more tests - especially for LinksResources.
Hence, I have now restructured tests/models.

It now uses pylint properly and loads and uses the test data via pytest fixtures.

ml-evs · 2020-06-16T21:30:35Z

Is this good to go now?

ml-evs · 2020-06-19T00:42:08Z

Is this good to go now?

I can't review my own PR (unfortunately), but I'm happy with all the changes here now if someone else would like to review it...

…n_types

site_attachments is a flag under structure_features. attached and nattached are fields for a species.

Currently it tests whether a species is represented in species_at_sites, however, implicit_atoms MUST also be set for some assemblies, this validation should be added.

It used to be a flag in structure_features, but has been replaced by implicit_atoms and site_attachments.

Several conversion functions for the StructureAdapter specifically handled None values in cartesian_site_positions, which is no longer an option, hence, these extra steps have been removed from the affected conversion functions.

These are optional properties for a LinksResource.

Using again test_more_structures.json revealed some errors with the new validators, as well as some updates that needed to be done to existing validators. In order to properly validate `species_at_sites` it has to be instantiated _after_ `species`.

Move to pytest. Add fixtures for loading test data.

ml-evs · 2020-06-19T15:17:43Z

This is really blocking other PRs now, I think @CasperWA is away at the moment, @shyamd or @fekad could you take a look if you get time?

ml-evs requested review from CasperWA, fekad and shyamd June 15, 2020 09:39

CasperWA changed the title ~~Add nperiodic_dimensions field~~ Update models according to changes during CECAM 2020 meeting Jun 15, 2020

CasperWA mentioned this pull request Jun 15, 2020

Improvements/fixes for openapi.json #332

Closed

3 tasks

ml-evs commented Jun 15, 2020

View reviewed changes

CasperWA force-pushed the ml-evs/nperiodic_dims branch from c5ef5c6 to a4d049a Compare June 15, 2020 22:50

CasperWA force-pushed the ml-evs/nperiodic_dims branch from e6d076c to 9f3a2b7 Compare June 16, 2020 00:12

ml-evs mentioned this pull request Jun 16, 2020

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

Merged

8 tasks

ml-evs force-pushed the ml-evs/nperiodic_dims branch from ab79168 to ef65ecc Compare June 16, 2020 21:32

ml-evs and others added 11 commits June 19, 2020 16:16

Added nperiodic_dimensions to all test inputs

cf29ae2

Added nperiodic_dimensions field to structure model

e54da12

Added bad structure data for mismatched nperiodic_dimensions/dimensio…

9992371

…n_types

Updated adapter tests with new field

1a2f073

Add attached and nattached + site_attachments

257b7c1

site_attachments is a flag under structure_features. attached and nattached are fields for a species.

Add implicit_atoms to structure_features

888a170

Currently it tests whether a species is represented in species_at_sites, however, implicit_atoms MUST also be set for some assemblies, this validation should be added.

unknown_positions is no longer

d345d79

It used to be a flag in structure_features, but has been replaced by implicit_atoms and site_attachments.

Add tests, remove handling of unknown positions

4004ae6

Several conversion functions for the StructureAdapter specifically handled None values in cartesian_site_positions, which is no longer an option, hence, these extra steps have been removed from the affected conversion functions.

Add aggregate and no_aggregate_reason

d852744

These are optional properties for a LinksResource.

Add type to entry listing info properties

48b1a4c

Add type field to all properties

e3c7175

CasperWA added 9 commits June 19, 2020 16:16

Use Enums for list of literal values

374f51c

Use enumerations for other literal values

037a76d

Add tests for DataType methods

5c66c12

Add more "bad" structures for testing validators

9386c95

Add new EOL for update OpenAPI tasks

c9c08b0

Use test_more_structures.json

39dc1eb

Using again test_more_structures.json revealed some errors with the new validators, as well as some updates that needed to be done to existing validators. In order to properly validate `species_at_sites` it has to be instantiated _after_ `species`.

Use Aggregate Enum for correct property ...

9a9acdb

Restructure tests/models

3bc77d3

Move to pytest. Add fixtures for loading test data.

Add LinksResource tests

5bce8e5

ml-evs force-pushed the ml-evs/nperiodic_dims branch from ef65ecc to 5bce8e5 Compare June 19, 2020 15:17

fekad approved these changes Jun 19, 2020

View reviewed changes

shyamd approved these changes Jun 19, 2020

View reviewed changes

shyamd merged commit b93610b into master Jun 19, 2020

CasperWA deleted the ml-evs/nperiodic_dims branch June 24, 2020 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update models according to changes during CECAM 2020 meeting #350

Update models according to changes during CECAM 2020 meeting #350

ml-evs commented Jun 15, 2020 •

edited by CasperWA

codecov bot commented Jun 15, 2020 •

edited

CasperWA commented Jun 15, 2020

CasperWA commented Jun 15, 2020 •

edited

CasperWA commented Jun 15, 2020

ml-evs Jun 15, 2020

CasperWA Jun 15, 2020 •

edited

CasperWA Jun 15, 2020

ml-evs Jun 15, 2020

ml-evs commented Jun 15, 2020

CasperWA commented Jun 15, 2020

CasperWA commented Jun 15, 2020

ml-evs commented Jun 16, 2020

ml-evs commented Jun 19, 2020

ml-evs commented Jun 19, 2020

Update models according to changes during CECAM 2020 meeting #350

Update models according to changes during CECAM 2020 meeting #350

Conversation

ml-evs commented Jun 15, 2020 • edited by CasperWA

codecov bot commented Jun 15, 2020 • edited

Codecov Report

CasperWA commented Jun 15, 2020

CasperWA commented Jun 15, 2020 • edited

CasperWA commented Jun 15, 2020

ml-evs Jun 15, 2020

Choose a reason for hiding this comment

CasperWA Jun 15, 2020 • edited

Choose a reason for hiding this comment

CasperWA Jun 15, 2020

Choose a reason for hiding this comment

ml-evs Jun 15, 2020

Choose a reason for hiding this comment

ml-evs commented Jun 15, 2020

CasperWA commented Jun 15, 2020

CasperWA commented Jun 15, 2020

ml-evs commented Jun 16, 2020

ml-evs commented Jun 19, 2020

ml-evs commented Jun 19, 2020

ml-evs commented Jun 15, 2020 •

edited by CasperWA

codecov bot commented Jun 15, 2020 •

edited

CasperWA commented Jun 15, 2020 •

edited

CasperWA Jun 15, 2020 •

edited