Skip to content

Conversation

@drammock
Copy link
Contributor

@drammock drammock commented Sep 17, 2025

This is needed for BEP042, specifically for bids-standard/bids-specification@5ea4b08 (#1998)

Code is copied (with minor typo fixes) from this forum answer so I've listed @rwblair in co-authored-by

marking as draft because I'm not 100% sure that just keys will allow us to check everything we want to check.

EDIT: approach changed after real-time convo with @effigies; now this collates multiple coordsystem files

xref

effigies added a commit to neuromechanist/bids-examples that referenced this pull request Sep 18, 2025
@effigies effigies changed the title add array of keys to associations.coordsystem feat: Add associations.coordsystems to collate coordsystem files Sep 19, 2025
@effigies effigies force-pushed the feat/add-keys-to-assoc-coordsys branch from 39d2bfc to 60b5168 Compare September 19, 2025 17:41
@drammock
Copy link
Contributor Author

drammock commented Sep 19, 2025

@effigies I'm doing local-run validation using the current state of this branch (e6de738). If I use the console.log(context) trick that Ross mentioned on the forum, I see this:

  subject: {},
  entities: { sub: "01", recording: "highDensity" },
  datatype: "emg",
  suffix: "electrodes",
  extension: ".tsv",
  modality: "emg",
  sidecar: {},
  associations: {},
  columns: Map(5) {
    "name" => [
   ... rest of context omitted

Notice that associations is empty. I think that means we're not capturing coordsystem.json files that have a space entity when electrodes.tsv does not have the space entity.

EDIT: this is on emg_concurrentIndependentUnits dataset. I think the empty associations is what is causing EMG_COORD_SYS_MISMATCH and EMG_COORD_SYS_PARENTS to fail for me locally

@drammock
Copy link
Contributor Author

Notice that associations is empty.

looks like you resolved it on the spec end. I guess this is ready for changelog and review/merge then?

Copy link
Contributor

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving some notes. This PR can be merged once @rwblair is happy with it, or remain open during the BEP42 review period. It would need to be merged (and the dev branch updated) before the examples PR can be merged.

extension,
rule.target.suffix,
rule.target?.entities ?? [],
).next().value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EMG examples do not have this case, but with multiple coordsystem.jsons, you could imagine having some at the root and some at the leaf.

We currently strongly assume (and implement with .next().value) that the first level found is the level of the association, which has worked for single-file associations. This probably needs to be relaxed, but the logic will be more intricate. I think you would need to take the first coordsystem.json with a given space entity, not the full collection.

For now this could be noted as a limitation in the validator implementation.

@codecov
Copy link

codecov bot commented Sep 25, 2025

Codecov Report

❌ Patch coverage is 30.23256% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.98%. Comparing base (fe09d89) to head (5cb3dc4).

Files with missing lines Patch % Lines
src/schema/associations.ts 30.23% 29 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
- Coverage   87.40%   86.98%   -0.43%     
==========================================
  Files          50       50              
  Lines        3677     3702      +25     
  Branches      603      604       +1     
==========================================
+ Hits         3214     3220       +6     
- Misses        453      472      +19     
  Partials       10       10              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@effigies effigies force-pushed the feat/add-keys-to-assoc-coordsys branch from c3c5bcd to 1fa78a6 Compare September 25, 2025 20:17
@effigies effigies changed the title feat: Add associations.coordsystems to collate coordsystem files BEP042 patches Sep 25, 2025
@effigies effigies marked this pull request as draft September 25, 2025 20:19
@effigies effigies added the bep label Oct 21, 2025
@effigies effigies force-pushed the feat/add-keys-to-assoc-coordsys branch 2 times, most recently from fe8a678 to c381d01 Compare October 21, 2025 15:21
@effigies effigies marked this pull request as ready for review October 21, 2025 15:24
@effigies effigies requested a review from rwblair October 21, 2025 15:25
@effigies effigies force-pushed the feat/add-keys-to-assoc-coordsys branch from 37209e3 to 5cb3dc4 Compare October 27, 2025 17:37
@rwblair rwblair merged commit eaced0d into bids-standard:main Oct 27, 2025
27 of 29 checks passed
@drammock drammock deleted the feat/add-keys-to-assoc-coordsys branch October 27, 2025 19:51
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.

3 participants