-
Notifications
You must be signed in to change notification settings - Fork 73
Update relationships #1298
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 relationships #1298
Conversation
BryanFauble
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.
1 comment but lgtm
Co-authored-by: BryanFauble <[email protected]>
| Defining data types: | ||
|
|
||
| - Put a unique data type name in the `Attribute` column. | ||
| - Put the value `DataType` in the `Parent` column. |
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 you confirm whether this step is required? If I remember correctly, the Parent column is only used for data visualization, so it shouldn’t be required.
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.
oh never mind. I tried without Parent column and got an error:
File "/Users/lpeng/code/synapsePythonClient/synapseclient/extensions/curator/schema_generation.py", line 606, in check_schema_definition
raise ValueError(
...<2 lines>...
)
ValueError: Schema extension headers: {'DependsOn', 'Format', 'Description', 'Source', 'DependsOn Component', 'Pattern', 'Valid Values', 'Maximum', 'Required', 'Minimum', 'Attribute', 'Properties', 'columnType', 'Validation Rules'} do not match required schema headers: ['Attribute', 'Description', 'Valid Values', 'DependsOn', 'Required', 'Parent', 'Validation Rules']
(
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.
It's required in the sense that it's how we differentiate between actual data types and other things that have attributes in the dependsOn column but are only there for conditional dependencies. If you go down to the Conditional Dependencies section example, Patient is an actual data type, but Cancer isn't. They both have
dependsOn values. So the way to tell them apart is by setting the Parent value. This is used by generate_json_schema when the user doesn't submit a data type, which means to create a JSON Schema for every data type.
It's also required in the sense that the DataModelrelationships class throws an error if the column isn't present in the CSV.
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.
I think for
Valid Values, theparentcolunm also needs to be filled
It doesn't for the purpose of creating JSON Schema (I just tried).
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.
Okay got it. Thank you for explaining.. Maybe we should just change Parent column to DataType and just fill it with True or False in the future.
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.
That would probably be the best way to do it if we were starting from scratch :)
linglp
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.
LGTM! Thank you for your hard work. I left some minor comments.
Problem:
The Data Model documentation wasn't quite accurate.
The
Parentcolumn wasn't described.The
Validation Rulescolumn wasn't listed as required.These columns were required by the
DataModelRelationshipsclass, but aren't for JSON Schema creation:DependsOn ComponentPropertiesSourceSolution:
The documentation has been updated.
These columns are no longer required:
DependsOn ComponentPropertiesSourceNote: I tried to make ValidationRules not required, but doing so broke a lot of tests. There may be a bug here. This will need to be resolved in SYNPY-1693