-
Notifications
You must be signed in to change notification settings - Fork 28
feat: update development_stage_ontology_term_id and development_stage for 7.0 #1428
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
e8eb2f4 to
cb62ed2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1428 +/- ##
==========================================
+ Coverage 88.99% 89.06% +0.06%
==========================================
Files 23 23
Lines 2580 2596 +16
==========================================
+ Hits 2296 2312 +16
Misses 284 284
🚀 New features to boost your workflow:
|
cb62ed2 to
8b782d4
Compare
| type: curie | ||
| curie_constraints: | ||
| ontologies: | ||
| - NA |
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.
Does this need to be lowercase na?
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 would also work if we changed it to na, but i think by convention, we just use capitalized NA to make it consistent with other real ontologies like UBERON or CL. there's a few other fields in the schema definition where we use NA as the ontology placeholder for the string na
kliu-czi
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.
Do we need to have a definition of what na is in this universe?
it just means "not applicable", which is different from "unknown" even though they might seem similar. this particular example is a bit hard to understand unless you already know what a cell line is, but if you take a look at the definition for self_reported_ethnicity_ontology_term_id it's easier to understand the difference between the two. so if the organism type isn't a human and it's a mouse cell, then it doesn't have a self reported ethnicity, therefore, it must be set to |
Reason for Change
https://czi.atlassian.net/browse/VC-3425
Changes
obs (Cell metadata)
note that in order to do this, i had to introduce a new keyword paradigm in the column dependencies,
exclude_exactandexclude_ancestors_inclusive, which are more or less the opposite of the existingmatch_exactandmatch_ancestors_inclusive. this is because when i first added the two tissue_type column dependency rules (one to require thatdevelopment_stage_ontology_term_idiftissue_type = 'na', and the other to require thatnais not allowed fordevelopment_stage_ontology_term_idiftissue_typeis notna, this meant that the rule enforcing "if organism is none of the above" wouldn't kick in in the case where say,tissue_type = tissueandorganism = not human, mouse, zebrafish, fruit fly, or roundwormbecause that rule only kicks in if none of the column-based rules match.i think this also helps make the schema definition more explicitly clear what the conditions are for the rule kicking in in the "else" case. another option i considered to handle this would be to rewrite the schema language such that for column dependencies, we have
if,elifandelselogic built in more explicitly rather than justiflogic. but that would be a bit more of an involved change with updating the_validate_column_dependenciesfunction to parse through that language and i think that's out of scope for now.Testing
test_cell_line_development_stage_ontology_term_idandtest_cell_line_cannot_be_na_for_tissueNotes for Reviewer