-
Notifications
You must be signed in to change notification settings - Fork 73
[SYNPY-1698] Add CSV data model tutorial #1292
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
thomasyu888
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.
🔥 Great document here outlining the context and all the columns to add and the relationships are very clear. Some minor comments
- change the title to "curator_data_model.md"
- Move this document into explanations folder as i think it falls under those guidelines better than the tutorial guidelines: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/docs/tutorials/README
- Add this page under Further reading section here: https://github.com/Sage-Bionetworks/synapsePythonClient/blob/develop/mkdocs.yml
Wait for it to build, then you can view the built page here: https://synapsepythonclient--1292.org.readthedocs.build/en/1292/. I think I will also Ask Jess V to review this, but if @linglp can provide a final review as well
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.
Hi @andrewelamb ! Thanks for your hard work. I think we're very close, but I have two concerns:
- Shift between two data types: We alternate between "Patient" and "Person" in our examples. Should we standardize on one?
- Attribute alignment: Some attributes in the "Person" examples seem misaligned and not truly Person-level properties. Could we revise those to use more appropriate person-level attributes?
- I noticed a few things in the conditional dependencies example that we might want to adjust. See my comments below.
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.
Thanks for your hard work, Andrew! This is looking better. But you might want to consider regenerating the JSON Schema in the examples using the updated data model. See my comments below!
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.
Pull request overview
This PR adds comprehensive documentation for creating CSV data models for the Curator extension (formerly Schematic), addressing SYNPY-1698. The documentation explains how to structure data models in CSV format that can be converted to JSON Schemas for use in Curator.
- Added new tutorial document explaining CSV data model structure and conversion to JSON Schema
- Integrated the new documentation into the navigation structure
- Provided extensive examples demonstrating each data model column and feature
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| mkdocs.yml | Added "Curator Data model" entry to the Explanations section of the documentation navigation |
| docs/explanations/curator_data_model.md | New comprehensive tutorial covering CSV data model structure, columns (Attribute, DependsOn, Description, Valid Values, Required, columnType, Format, Pattern, Minimum/Maximum), conditional dependencies, and multiple detailed examples with corresponding JSON Schema outputs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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! Just some minor comments.
|
@thomasyu888 Should I merge this, or do you want to let Jess take a look first? |
thomasyu888
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! @andrewelamb let's merge and I'll ship this to Jess after the merge
SYNPY-1698
Problem:
The documentation is missing how to create the data model CSV for Curator extension.
Solution:
An updated doc has been added.
https://synapsepythonclient--1292.org.readthedocs.build/en/1292/