-
Notifications
You must be signed in to change notification settings - Fork 169
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
[ENH] Add descriptions.tsv
file relating to the desc-<label>
entity
#1613
Conversation
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.
updated the source file
This could be extended to most label-based entities, right? E.g., Asking this because reframing this PR as a general-purpose "label entity-describing TSVs are allowed" enhancement would be useful across raw and derivative datasets and would also circumvent any need to reference a specific BEP. |
Good point, though this may need to also take into account some of entity-label-descriptors metadata precedents we already have:
|
@tsalo the idea here is that -- but it's true that some values (numeric or txt) are a little under specified |
I think it does apply to |
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 am fine with this 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.
should we schemafy this somehow? The prescription about desc_id
and description
as mandatory columns in an optional descriptions.tsv
seem like something that should live in the schema.
d20b5d1
to
248d261
Compare
thx stefan! |
@sappelhoff is it good to merge? the pdf builds still fails though |
I'm still not sure whether we need to put some of this into the schema: #1613 (review) what do you think @bids-standard/maintainers? |
I would say yes but in a separate PR?
|
descriptions.tsv
file relating to the desc-<label>
entity
1ed3d05
to
4b613a6
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
4b613a6
to
777064c
Compare
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 @Remi-Gau I agree that this can be done in a separate PR.
I have gone over the text and streamlined it. I also removed one part.
Please check, and if all of us agree now, we can merge it.
777064c
to
860d427
Compare
label makes more sense 👍 |
I figured that I believe that this entity-to-tsvfile-mapping would be a good general principle (like an SQL primary key) that we could also have used for other entities, such as (*) Note that for consistency subjects.tsv would have been better than participants.tsv. |
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 guess @robertoostenveld was right, once again! good for me
Thanks @robertoostenveld, I completely forgot about that. changed it back in fede5d3 should be all ready now. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1613 +/- ##
==========================================
+ Coverage 87.83% 87.97% +0.14%
==========================================
Files 16 16
Lines 1356 1356
==========================================
+ Hits 1191 1193 +2
+ Misses 165 163 -2 ☔ View full report in Codecov by Sentry. |
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 all, looks good to me.
add descriptions.tsv file as optional based on the derivatives guidelines https://docs.google.com/document/d/1JtTu5u7XTkWxxnCIH6sxGajGn1qG_syJ-p14aejpk3E/edit?usp=sharing