-
Notifications
You must be signed in to change notification settings - Fork 188
[ENH] Clarify that recording entity may be used to distinguish recording instruments #2090
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] Clarify that recording entity may be used to distinguish recording instruments #2090
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2090 +/- ##
=======================================
Coverage 82.71% 82.71%
=======================================
Files 20 20
Lines 1608 1608
=======================================
Hits 1330 1330
Misses 278 278 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This sounds like the exact use case for the At least in the MRI world, two files that differed by |
We can expand the definition both as a separate PR or directly on #1998. |
Yes, it was discussed here: #1998 (comment). To set the scene, in some EMG experiments, separate EMG systems are being used at the same time, recording with different settings, parameters, starting times, and sensor locations. We intend to use Here are the current definitions:
A couple of points in favor of using I am in favor of slightly expanding definitions rather than introducing another entity, although that could also work. |
I thought there had been some discussion of using
Does that prevent using it in other ways though? Would it be wrong for a dataset to contain, for example
I personally don't care much whether we use |
I don't think so. If we're considering expanding I think we could update the
Would that cover your use case? Then it's just a matter of EMG files declaring It also looks like BEP 020 (#1128) is using I'm not dead set against expanding |
agreed
agreed.
Yep I think so. @neuromechanist do you agree that this would suffice? |
TL, DR: BIDS had provided the Interestingly, the issue's scope is becoming much larger than anticipated, so I appreciate making it a separate pull request. I see that either entities would require expansion, and with @drammock's example, using
I have reservations about this statement. Auxiliary recordings are auxiliary not in the sense that it might be important for someone or not, but in the sense that which IMO, we would benefit from using two separate entities to distinguish if the data file is auxiliary (as they live under a different The use-case could become even more potent when considering the new BEP045 (physio-BIDS, #1675, cc @smoia), where the Currently, Motion-BIDS (cc @JuliusWelzel, @sjeung) use I also understand that this change has BIDS2.0 implications, both about scans/recordings.tsv and modality independence bids-standard/bids-2-devel#29 and bids-standard/bids-2-devel#45 (cc @tsalo, @yarikoptic) and also potentially harmonizing the To sum up, I think any or all will work ok, so I won't have any objections against them. Using different entities may make the file structures more clear down the road, and I hope that I could lay out the case and involve all potential stakeholders. |
Yes, but the use of |
@drammock would it be possible to add some examples of this definition extension of |
@neuromechanist @drammock in #1675 and BEP045 we're proposing to add an optional metadata field to indicate what data are collected together and hence are "auxiliary" (without necessarily forcing a hierarchy). IDK if that could be a valid option for you as well? Also cc'ing other BEP045 leads startign with @m-miedema |
re Left a comment with my preference on #1998 (comment). As for "auxiliary" data types -- I think we should at large strive for making all data types "1st class citizens" since discrimination between them is mostly "an intent" of an experimenter and thus IMHO should be just noted at the level of the dataset, and otherwise result in identical layouts regardless of the intent: users looking for datatype X should find it in a consistent location regardless of the intent of the experimenter, and then some metadata structure (e.g. within filenames and |
@smoia says:
That looks useful, but I don't think it captures/addresses the use case we're facing. We're dealing with EMG as the primary modality, but with multiple separate devices making recordings simultaneously (from different muscles/locations on the body). So we need multiple @yarikoptic says that we should consider the |
It does help, however the suffix is
assuming that the two recordings have different sampling frequencies. Otherwise, they can be sibling columns in a single With the acceptance of #1128 (fingers crossed), we could quickly define a new |
@oesteban Please see the original description of this PR, where in the first sentence I explain that:
I intentionally didn't include an example here at first, since I know that it wouldn't make sense to merge an example that illustrates the EMG modality before that BEP is merged; I added such an example later at your request. So if you assume for a moment that an EMG modality does exist (or will soon) and interpret the example in that context, what do you think? Is expanding the definition of
|
I did read the initial message. But rather than a solution to a problem
within that BEP, it is presented as a current problem of BIDS.
Unless an example fully compliant with current BIDS is given, I can only
see this as a solution to an issue we don't have, and we may not have it in
the future either.
The eye tracking BEP020 avoided creating new suffixes and would be solid
alternative to represent physio data with minimal changes to BIDS (no new
entities).
…On Tue, Apr 1, 2025, 16:45 Daniel McCloy ***@***.***> wrote:
@oesteban <https://github.com/oesteban> example added in 034f62a
<034f62a>.
Does that help?
It does help, however the suffix is _emg which doesn't exist in BIDS to
date.
@oesteban <https://github.com/oesteban> Please see the original
description of this PR, where in the first sentence I explain that:
In #1998 <#1998>
we're working on a BEP for EMG data.
I intentionally didn't include an example here at first, since I know that
it wouldn't make sense to merge an example that illustrates the EMG
modality *before* that BEP is merged; I added such an example later at
your request. So if you assume for a moment that an EMG modality does exist
(or will soon) and interpret the example in that context, what do you
think? Is expanding the definition of acq good (as in this PR), or do you
prefer one of the other suggestions that have emerged:
- use recording
- use a new entity device
- use a new entity sys (based on tracksys)
—
Reply to this email directly, view it on GitHub
<#2090 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAESDRTZNRODHGEWOWH4OOD2XKYATAVCNFSM6AAAAABZVLGZBWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRZHAYTOOBQHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
[image: drammock]*drammock* left a comment
(bids-standard/bids-specification#2090)
<#2090 (comment)>
@oesteban <https://github.com/oesteban> example added in 034f62a
<034f62a>.
Does that help?
It does help, however the suffix is _emg which doesn't exist in BIDS to
date.
@oesteban <https://github.com/oesteban> Please see the original
description of this PR, where in the first sentence I explain that:
In #1998 <#1998>
we're working on a BEP for EMG data.
I intentionally didn't include an example here at first, since I know that
it wouldn't make sense to merge an example that illustrates the EMG
modality *before* that BEP is merged; I added such an example later at
your request. So if you assume for a moment that an EMG modality does exist
(or will soon) and interpret the example in that context, what do you
think? Is expanding the definition of acq good (as in this PR), or do you
prefer one of the other suggestions that have emerged:
- use recording
- use a new entity device
- use a new entity sys (based on tracksys)
—
Reply to this email directly, view it on GitHub
<#2090 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAESDRTZNRODHGEWOWH4OOD2XKYATAVCNFSM6AAAAABZVLGZBWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDONRZHAYTOOBQHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
OK, that was not my intent, so apologies that the original message wasn't clear. Hopefully this clarifies: We have enountered a problem in preparing a BEP for electromyography (cf #1998) that we want to get wider input on. It is common to record EMG at different sites from multiple devices simultaneously. Sometimes the devices feed into the same amplifier and the data end up in the same file, sometimes that is not the case. When it's not, we need an entity to distinguish data files from devices recording simultaneously. We're considering using
Perhaps. But this PR as written isn't proposing any new entities, it's proposing to (slightly) expand the definition of an existing entity. @effigies then suggested using If your feedback is "there is no need for an EMG modality, just use |
If this PR is merged but BEP042 finally doesn't use it or uses it differently, then we would be adding something already abandoned with this PR. I believe this should be discussed there already. Happy to join the discussion of BEP042 and make a case for the BEP020 rationale. That said, I'll try to push BEP020 over the finish line before I can dedicate any time to another physiology BEP. |
FWIW, I also support the @oesteban, You are more than welcome to join the BEP042 #1998 or #1371 discussion. I look forward to discussing the possibility of including electromyography on different limbs as a part of Eye-tracking specifications. An argument might be whether there is a need for separate Eye-tracking or EMG BEPs, and shouldn't all be consolidated into BEP045 (Physio-BIDS, #1675). Although this discussion has been done here and there, it might be worth consolidation in a separate Issue. IMO, the rationale for this PR is quite clear and beyond #1998 as comments provided by the stakeholders from other BEPs also point out: Some modalities might have concurrent data recordings that, for some technical reasons, can't be part fo the same file. This can be different tracking systems in Motion-BIDS, concurrent video recordings in #1771, multiple Physio recordings in BEP045 and also recording EMG using more than one device. Since these efforts are going quite in parallel, it made sense to create this PR to coordinate with all stakeholders to harmonize the use of an entity that refers to such concurrent recording across all data types/modalities.
Yes, but that is quite minimal. Even in that case, there are still several other potential BEPs that could use this PR for their specifications. |
Sure, that's not what's being proposed here, and considering the volume of such a change, a device entity would likely require its own BEP because it conflicts with current BIDS foundations (which may be wrong, I'm not entering that debate). Let's focus on this particular PR.
Thank you for the invitation—I certainly appreciate the openness to broader discussions. My main point here was procedural clarity: the BEP mechanism is designed precisely to handle these broader, cross-cutting discussions. BIDS established the BEP mechanism for large changes so those iterations over the spec are made consistently. Splitting this from those BEPs is inconsistent and risks introducing a problem. If there's a dependency relationship between BEPs (and that sounds reasonable and likely), then the solution is to make BEP045 contingent on the outcome of BEP042, so if this change to acq- is accepted there, then BEP045 would just piggyback on that change. As you indicate at the end, I would not call for caution if this were just a separate issue—however, it's a PR that may introduce a change to the spec.
I understand and appreciate that rationale. The issue I'm raising here is one of scope and procedure. The PR mechanism in BIDS is typically reserved for incremental, less controversial changes or bug fixes. Given the cross-cutting nature of the proposed change (affecting multiple BEPs), addressing this change first as an issue calling for opinions or as part of BEP042 seems more appropriate. This would ensure that all stakeholders have a clear understanding and agreement before making permanent updates to the specification. In general, BIDS evolves the spec BEP-by-BEP. The PR mechanism is reserved to fix uncontroversial, safe changes.
I completely agree and support the idea of coordination and harmonization across BEPs—it is indeed essential. My intention isn't to discourage coordination but rather to ensure it happens in a structured way that respects established BIDS procedures. Perhaps an issue explicitly aimed at harmonizing these parallel discussions, gathering input from all stakeholders upfront, might serve better than immediately proceeding with a specification-changing PR.
I appreciate your assessment here. My perspective is simply that any risk—minimal as it may seem—of prematurely committing to a specification change that could affect ongoing BEPs warrants caution. It's precisely to manage this kind of risk that BIDS established the BEP mechanism. Overall, my recommendation is procedural rather than conceptual: let's ensure we're clear on dependencies, implications, and cross-BEP coordination before committing to this PR. I believe that will strengthen our collective efforts, reduce potential friction, and better serve the community in the long run. |
I appreciate you taking the time to clearly explain your rationale @oesteban.
That makes sense to me, and I think the root of this dialog is that I am used to a different set of norms around the distinction between "issue" and "pull request". In the dev communities where I spend most of my time, it's OK to open pull requests as a conversation-starter/request-for-input if it makes it easier to see exactly what is being suggested (via the diff view on GitHub). I probably should have made it clearer that such was my intent (at a minimum marking the PR as draft, which I'll do now).
I'm happy to work within the norms of this community, now that I know what some of them are. |
Thanks for clarifying! I completely understand where you're coming from. BIDS BEPs function similarly to Python's PEPs (and other large-scale projects), guiding broader discussions through established processes to ensure changes have solid consensus. Given how critical backward compatibility is for BIDS, each PR tends to be carefully evaluated. Even changes that appear straightforward at first glance can have unforeseen implications later on, which is why careful review is so important.
I’m sorry if my feedback came across as harsh—this was definitely not my intent. I understand your approach better now after seeing how this discussion started within the BEP. For those like me who had not engaged in the BEP but received notifications of this PR, it seemed to represent an effort to push a change rather than a request for discussion. And as I mention above, changes to BIDS require rigorous assessment even if they seem innocuous. Marking it as a draft now does help clarify your intent, and I appreciate you doing that. I will try to look at BEP042 asap and provide feedback. That said, we are in the final stages of BEP020 and I think that could definitely change the way BEP042 and BEP045 are being approached. I would invite you, @neuromechanist, @JuliusWelzel and others to join our meeting on Thursday (info to join is given on the PR's message #1128). |
I brought this PR to today's @bids-standard/maintainers. It seems that Maybe harmonizing the entity to indicate recordings from different instruments ( |
I think the overall consensus is to use the The definition can be very minimally updated: bids-specification/src/schema/objects/entities.yaml Lines 253 to 255 in 034f62a
This can be updated 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.
Should this still be marked draft?
thanks for the ping. I've updated the PR description slightly to match the edited title, and pulled out of draft mode. |
In #1998 we're working on a BEP for EMG data. We're hoping to use the
acq
entity for cases where multiple EMG devices were used simultaneously, but they were recording through different amplifiers, into different data files, possibly with different sampling frequencies, etc. This PR proposes some slight wording changes totheEDIT: following #2090 (comment) we're going to useacq
entity definition to make it clearer that such uses ofacq
are acceptable.recording
instead, and this PR modifies the definition ofrecording
accordingly.(if they're deemed not acceptable, better that we know that now so we can adapt #1998 accordingly).
One possible alternative would be a new
sys
entity, analogous to thetracksys
entity introduced for BIDS-Motion, with the (likely) end result that in future releasestracksys
would be replaced bysys
in motion datasets.cc @JuliusWelzel @neuromechanist