-
Notifications
You must be signed in to change notification settings - Fork 32
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
Added enzymatic tissue dissociation fields #1599
base: staging
Are you sure you want to change the base?
Conversation
…nto ac-tissue-dissociation-Issue1592
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.
Minor comments and a change to the regex, otherwise LGTM!
json_schema/type/protocol/biomaterial_collection/dissociation_protocol.json
Outdated
Show resolved
Hide resolved
Co-authored-by: ESapenaVentura <[email protected]>
Co-authored-by: ESapenaVentura <[email protected]>
…nto ac-tissue-dissociation-Issue1592
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 👍
}, | ||
"digestion_temperature": { | ||
"description": "Temperature of digestion in Celsius or in general terms (i.e. cold, warm, room temperature).", | ||
"pattern": "^(\\d+(\\.\\d+)?|frozen|cold|room temperature|warm)$", |
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.
The number pattern matches non-sensical strings such as 00000000021.210000000000. Consider a union (anyOf
) of number
and enum
.
Then again, instead of allowing the subjective English terms, you could just allow for a range. That might be more elegant and easier to process for consumers.
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.
Thank you for your comment, I agree on using range instead of words however, the Lung bionetwork insisted on using words to record this, as they used in the published version 1 of their atlas.
I've added the solution with the union of types in 0944f6e .
"Protease", | ||
"TrypLE", | ||
"Trypsin" | ||
] |
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 bracket seems misaligned.
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 pointing that out, fixed in 0944f6e
}, | ||
"digestion_temperature": { | ||
"description": "Temperature of digestion in Celsius or in general terms (i.e. cold, warm, room temperature).", | ||
"anyOf": [ |
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 was confused but
"anyOf": [ | |
"oneOf": [ |
is semantically equivalent (given that the alternatives are disjunctive) and adds clarity.
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!
#1592
Release notes
For
dissociation_protocol.json
schema:reagents
friendly name to distinguish withdigestion_solution
digestion_time
digestion_time_units
digestion_temperature
digestion_solution
Reviews requested