-
Notifications
You must be signed in to change notification settings - Fork 42
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 models according to changes during CECAM 2020 meeting #350
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Exactly this is related to the issue #198. |
Changes to models:
Changes to code:
|
We are currently only testing the validators and validity of the structures models explicitly. |
@classmethod | ||
def from_python_type(cls, python_type: Union[type, str, object]): | ||
"""Get OPTIMADE data type from a Python type""" | ||
mapping = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Agreed, one for #104, I guess... |
c5ef5c6
to
a4d049a
Compare
I've taken care of the merge conflicts with the test structures and found that we weren't actually testing I had to change the initialization of We apparently validated that a |
We need to make some more tests - especially for It now uses pylint properly and loads and uses the test data via pytest fixtures. |
e6d076c
to
9f3a2b7
Compare
Is this good to go now? |
ab79168
to
ef65ecc
Compare
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... |
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.
ef65ecc
to
5bce8e5
Compare
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