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

[ENH] Add descriptions.tsv file relating to the desc-<label> entity #1613

Merged
merged 14 commits into from
Nov 9, 2023

Conversation

CPernet
Copy link
Collaborator

@CPernet CPernet commented Sep 13, 2023

add descriptions.tsv file as optional based on the derivatives guidelines https://docs.google.com/document/d/1JtTu5u7XTkWxxnCIH6sxGajGn1qG_syJ-p14aejpk3E/edit?usp=sharing

Copy link
Collaborator Author

@CPernet CPernet left a 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

@tsalo
Copy link
Member

tsalo commented Sep 15, 2023

This could be extended to most label-based entities, right? E.g., ses, rec, acq. It seems like we could have a whole suite of top-level entity-describing TSVs, equivalent to the participants.tsv file.

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.

@Remi-Gau
Copy link
Collaborator

This could be extended to most label-based entities, right? E.g., ses, rec, acq. It seems like we could have a whole suite of top-level entity-describing TSVs, equivalent to the participants.tsv file.

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:

  • task --> TaskName and TaskDescription
  • res --> Resolution
  • den --> Density
  • space --> SpatialReference
  • ce --> ContrastBolusIngredient
  • trc --> several tracer related metadata
  • ...

@CPernet
Copy link
Collaborator Author

CPernet commented Sep 15, 2023

@tsalo the idea here is that desc- being free form, we need only that entity for many things, you can see in the example desc-Filt, desc-FiltDs, desc-preproc and then we define the values. This does not really apply to other entities, you never have in the same folder files that concatenated sessions or reconstruction, etc ...

-- but it's true that some values (numeric or txt) are a little under specified

@tsalo
Copy link
Member

tsalo commented Sep 15, 2023

@tsalo the idea here is that desc- being free form, we need only that entity for many things, you can see in the example desc-Filt, desc-FiltDs, desc-preproc and then we define the values. This does not really apply to other entities, you never have in the same folder files that concatenated sessions or reconstruction, etc ...

I think it does apply to acq (especially after curating a dataset with variant-labeling tools like CuBIDS), rec, proc, and possibly recording, but you and @Remi-Gau are absolutely right that most of the entities I was thinking of are better constrained/labeled by metadata than I realized. I still think this new PR leads naturally to an acquisitions.tsv and a reconstructions.tsv in raw datasets, and possibly a processings.tsv (terrible name) and a recordings.tsv.

@robertoostenveld robertoostenveld self-requested a review October 2, 2023 07:29
Copy link
Collaborator

@robertoostenveld robertoostenveld left a 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.

Copy link
Member

@sappelhoff sappelhoff left a 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.

@sappelhoff sappelhoff force-pushed the descriptions branch 2 times, most recently from d20b5d1 to 248d261 Compare October 24, 2023 19:05
@CPernet
Copy link
Collaborator Author

CPernet commented Oct 24, 2023

thx stefan!

@CPernet
Copy link
Collaborator Author

CPernet commented Nov 6, 2023

@sappelhoff is it good to merge? the pdf builds still fails though

@sappelhoff
Copy link
Member

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?

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Nov 7, 2023

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?

@sappelhoff sappelhoff changed the title add descriptions.tsv [ENH] Add descriptions.tsv file relating to the desc-<label> entity Nov 7, 2023
Copy link
Member

@sappelhoff sappelhoff left a 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.

@CPernet
Copy link
Collaborator Author

CPernet commented Nov 8, 2023

label makes more sense 👍

@robertoostenveld
Copy link
Collaborator

label makes more sense

I figured that desc_id would make it consistent with other tsv files that accompany entities, such as sub->participants.tsv (see * note below), session->sessions.tsv, sample->samples.tsv.

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 task (which is now detailled as TaskDescription in a json), trc (which is now detailed as TracerName in a json), and probably a few others. It is something I would like to see in BIDS 2.

(*) Note that for consistency subjects.tsv would have been better than participants.tsv.

Copy link
Collaborator Author

@CPernet CPernet left a 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

@sappelhoff
Copy link
Member

Thanks @robertoostenveld, I completely forgot about that. changed it back in fede5d3

should be all ready now.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4096382) 87.83% compared to head (fede5d3) 87.97%.
Report is 3 commits behind head on master.

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     

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@robertoostenveld robertoostenveld left a 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.

@sappelhoff sappelhoff merged commit bf015e6 into bids-standard:master Nov 9, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants