-
Notifications
You must be signed in to change notification settings - Fork 4
Improves ClassList integration with Pydantic and adds JSON conversion for Projects
#79
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
Conversation
DrPaulSharp
left a comment
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.
This is really outstanding work, and will help us hugely going forwards.
I have a few comments and questions. Of particular importance is the issue regarding coercion for ClassLists - I'm afraid coercion is a many-tentacled monster.
I'd also like you to review the test coverage please. There's the particular point I've raised in test_convert.py, where there are also a few other lines missing in the coverage.
Finally, recalling the conversation we had prior to your leave about the principle of having the ClassList as a general, transferrable type, it'd be good to have tests for coercion done in test_classlist, probably within a "test_pydantic features" class. Of course, we should establish the exact behaviour we want first.
DrPaulSharp
left a comment
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.
All looks good, just a bit of tidying up to do. In addition to the specifics below, could you please review check_contrasts in project.py and make it look more like the check_layers routine. Specifically, add a docstring, add the additional line to the error message and correct the mention of layers in the error message please.
DrPaulSharp
left a comment
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.
This all looks good, make sure to merge with pride! (I think that involves playing a fanfare or something)
This PR solves #76 by allowing
Projects to be read and written from JSON via two new functions,project_to_jsonandproject_from_json.Changes
ClassListis now aGenericclass, improving compatibility with static type checkers (e.g. you can now writeClassList[Parameter], and type checking will know a priori that theClassListshould haveParameters in it)ClassLists, which means that Pydantic can validate them automatically. The type of theClassListis specified using square brackets, e.g. aClassListof strings would beClassList[str]. This schema does the following:ClassList, and raise aValidationErrorif the list types are not homogeneousClassList[T].dataas though it were alist[T]type, i.e. ensure that all entries in theClassListare the type expected by the model.Projectrather than aClassListand it will be converted automaticallyClassListcan be given as dictionariesFor example, the following are now all valid inputs to a Project:
In future, once Pydantic has integrated Numpy support (pydantic/pydantic#9677), this will be good enough for us to use Pydantic's
model_dump_jsonto saveProjects in JSON format (via converting eachClassListofRATModels to a list of dictionaries, as it now knows how to get back from that). But for now:convert_to_jsonandconvert_from_jsonwhich convert aProjectto and from JSON format, with workarounds to handleClassLists and the Numpy array in theDatamodel.