-
Notifications
You must be signed in to change notification settings - Fork 159
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
[SCHEMA] Add MeasurementToolMetadata and Derivative metadata fields #899
Conversation
Both fields are described in the Phenotypic data section of the spec.
src/02-common-principles.md
Outdated
@@ -573,6 +573,7 @@ using an object containing the following fields: | |||
"Units": "RECOMMENDED", | |||
"TermURL": "RECOMMENDED", | |||
"HED": "OPTIONAL", | |||
"Derivative": "OPTIONAL", |
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'm not sure if we want this here, since it's unclear to me how prevalent this field is. The field is only mentioned once, in Phenotypic and assessment data, but it says it can be used in participants.json
, so it definitely extends beyond that one section.
Here's the text:
In addition to the keys available to describe columns in all tabular files
(LongName
,Description
,Levels
,Units
, andTermURL
) the
participants.json
file as well as phenotypic files can also include column
descriptions with aDerivative
field that, when set to true, indicates that
values in the corresponding column is a transformation of values from other
columns (for example a summary score based on a subset of items in a
questionnaire).
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.
Wow ... I wasn't even aware that we had a Derivative
field 🤦♂️ In my experience it's the opposite of prevalent (never saw it, never heard of it)
Is there a way with the schema to contain it in that section? I.e., leave it only valid for JSON files nested in a phenotype
dir ? --> phenotype/<measurement_tool_name>.json
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.
At the moment, we still don't have rules like that in the schema, so that's something that would have to happen on the validator's side.
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.
Wow ... I wasn't even aware that we had a
Derivative
field man_facepalming In my experience it's the opposite of prevalent (never saw it, never heard of it)
Never saw it either. Would it make sense to include in the json example in the phenotype section (in a different PR) ?
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.
@Remi-Gau That sounds like a good idea to me.
@sappelhoff One thing that I failed to consider in my initial response to your question is that the spec does say that Derivative
can be used in participants.json
, so I think it would violate the spec to restrict Derivative
to phenotype files in the validator. At the moment, my main concern is whether we should "advertise" Derivative
by including it in the Tabular files section's metadata table. The more I think about it, the more I think I shouldn't include it in this table- at least until we've discussed the field a bit more elsewhere (e.g., in a separate issue). WDYT?
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.
Yes, agreed. I think making Derivative
"visible" in the right tables AND in the examples would best be done in a dedicated PR that also streamlines existing text.
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.
Sounds good. I'll remove it from this table in this PR, and open an issue for adding it in another one.
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.
Let's continue that discussion in #911, and then follow up with a PR.
In this PR, adding Derivative
to the schema should be enough (next to the other changes, which are good)
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.
ah @tsalo I already opened an issue :-) please chime in with your thoughts there, thanks for your work and digging this out!
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, thanks @tsalo --> we just need to finish discussion on the funny "Derivative" field
src/02-common-principles.md
Outdated
@@ -573,6 +573,7 @@ using an object containing the following fields: | |||
"Units": "RECOMMENDED", | |||
"TermURL": "RECOMMENDED", | |||
"HED": "OPTIONAL", | |||
"Derivative": "OPTIONAL", |
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.
Wow ... I wasn't even aware that we had a Derivative
field 🤦♂️ In my experience it's the opposite of prevalent (never saw it, never heard of it)
Is there a way with the schema to contain it in that section? I.e., leave it only valid for JSON files nested in a phenotype
dir ? --> phenotype/<measurement_tool_name>.json
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.
good to go from my side, once Derivative
is removed from the table again, let's discuss that in #911 and merge this one soon
Given that this is a fix, rather than an enhancement, I'm going to merge now instead of waiting five days. |
Closes None, but adds fields that we previously missed.
Changes proposed: