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

[FIX] Clarify run entity to accommodate multiple imaging modalities #760

Merged
merged 3 commits into from
Apr 6, 2022

Conversation

yarikoptic
Copy link
Collaborator

As we have now other modalities, such as EEG and MEG, "scan" is not really the
most appropriate term here. Having said that we do use _scans etc for EEG and
MEG as well ATM, as it was the "forward compatible" with existing MRI-centric
BIDS back then.

@effigies
Copy link
Collaborator

My intuition would be that a "data sample" is a volume, while "recording" would cover a time series. Not sure how other modalities would think about it.

This isn't an objection, just trying to see if there's a clearer word. I'm okay with "data sample" if it's clear to most people.

@yarikoptic yarikoptic mentioned this pull request Mar 17, 2021
@yarikoptic
Copy link
Collaborator Author

FWIW, I am not entirely happy with "data sample" either for the reasons you have stated. May be someone could suggest a better name...

@sappelhoff
Copy link
Member

I think I would find "recordings" most intuitive. But I don't think "scans" is a problem either - in particular because we do use scans.tsv for (structly speaking) non-scanning modalitites (EEG, ...)

-If several scans with the same acquisition parameters ...
+If several recordings (e.g., MRI scans) with the same acquisition parameters ...

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.

this is a relatively minor thing, but perhaps some more @bids-standard/maintainers have an opinion so that we can address this?

@sappelhoff sappelhoff added the opinions wanted Please read and offer your opinion on this matter label Nov 29, 2021
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@sappelhoff
Copy link
Member

FYI, I rebased on master and force pushed.

@tsalo
Copy link
Member

tsalo commented Mar 29, 2022

I agree that recording is a bit clearer than data sample, but the definition of "run" under common principles explicitly links it to "data acquisition". Should we use that instead?

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #760 (8bcaba2) into master (7ec7450) will increase coverage by 0.96%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
+ Coverage   70.53%   71.50%   +0.96%     
==========================================
  Files           9        9              
  Lines         930      930              
==========================================
+ Hits          656      665       +9     
+ Misses        274      265       -9     
Impacted Files Coverage Δ
tools/schemacode/schemacode/tests/test_schema.py 100.00% <0.00%> (ø)
tools/schemacode/schemacode/_version.py 38.90% <0.00%> (+2.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67c3d90...8bcaba2. Read the comment docs.

@sappelhoff sappelhoff merged commit e140f35 into bids-standard:master Apr 6, 2022
@sappelhoff
Copy link
Member

fyi @yarikoptic we discussed this in yesterday's maintainers meeting and as all suggestions to date were improvements, we just decided to just go with one of them.

@sappelhoff sappelhoff changed the title [FIX] run entity - "scans" -> "data samples" [FIX] run entity - "scans" -> "data acquisitions" Apr 6, 2022
@sappelhoff sappelhoff removed the opinions wanted Please read and offer your opinion on this matter label Jul 27, 2022
@effigies effigies changed the title [FIX] run entity - "scans" -> "data acquisitions" [FIX] Clarify run entity to accommodate multiple imaging modalities Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants