Skip to content
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

new expset property #1908

Merged
merged 2 commits into from
Sep 12, 2024
Merged

new expset property #1908

merged 2 commits into from
Sep 12, 2024

Conversation

RahiNav
Copy link
Contributor

@RahiNav RahiNav commented Sep 10, 2024

Added new property "disease_name" to experiment_set schema. It will use the ontology term already in 4DN most likely from MONDO (via EFO).

Copy link
Member

@aschroed aschroed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments that I think merit consideration and potential changes so I will wait to officially approve until you consider.

Also you might as well add an example to the 'tests/data/inserts/experiment_set.json' - you don't need to create a new item just add this property and an approriate term to an exising item.

@@ -97,6 +97,13 @@
"lookup": 20,
"formInput": "textarea"
},
"disease_name": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion to call this disease_term - helps convey it is a link to an OntologyTerm.

"disease_name": {
"title": "Disease Name",
"description": "Ontology term for the disease name associated with the set.",
"type": "string",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest making this property an array field so more than one disease/disorder term can be added.

@@ -97,6 +97,13 @@
"lookup": 20,
"formInput": "textarea"
},
"disease_name": {
"title": "Disease Name",
"description": "Ontology term for the disease name associated with the set.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the description is OK but maybe add 'disorder' as not everything is a 'disease' i.e. for the disease or disorder ...

@RahiNav
Copy link
Contributor Author

RahiNav commented Sep 12, 2024

@aschroed Great suggestions. I have made the updates. Let me know if it makes sense.

Copy link
Member

@aschroed aschroed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@RahiNav RahiNav merged commit a5ec62a into master Sep 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants