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

Completely rewrite HED support #2014

Merged
merged 4 commits into from
Jul 11, 2024
Merged

Conversation

happy5214
Copy link
Collaborator

This commit completely rewrites the HED integration module to only build most objects when necessary (due to memory issues).

This depends on code in hed-validator that has not yet been released to npm, and thus it currently does not build. Please test using the modify-bids-interface branch of hed-validator and npm link.

happy5214 added 3 commits July 5, 2024 10:47
This commit completely rewrites the HED integration module to only
build most objects when necessary (due to memory issues).
Copy link
Member

@VisLab VisLab left a comment

Choose a reason for hiding this comment

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

This looks great --- and follows as discussed in the code walk through.

@rwblair -- this update should also allow the HED validator to be used more easily in the deno version. (The current deno version may not actually validate HED.)

This version will require package lock to hed-validator v3.15.0 (which @happy5214 is in the processing of releasing on npm).

Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 86.11111% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.77%. Comparing base (53fdba6) to head (b2024d9).
Report is 2 commits behind head on master.

Files Patch % Lines
bids-validator/validators/hed.js 85.71% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2014      +/-   ##
==========================================
+ Coverage   85.68%   86.77%   +1.09%     
==========================================
  Files          91      132      +41     
  Lines        3792     6346    +2554     
  Branches     1220     1522     +302     
==========================================
+ Hits         3249     5507    +2258     
- Misses        457      748     +291     
- Partials       86       91       +5     

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

@happy5214 happy5214 marked this pull request as ready for review July 10, 2024 17:06
@effigies
Copy link
Collaborator

The ieeg_epilepsy_ecog example is failing, presumably because it has "HEDVersion": "n/a" in its dataset_description.json. Will open a PR to remove that, unless you'd rather it stay?

effigies referenced this pull request in bids-standard/bids-examples Jul 11, 2024
I believe this the reason that https://github.com/bids-standard/bids-validator/pull/2014 is failing. In any case, there's no advantage to having optional keys with empty or invalid contents.
@VisLab
Copy link
Member

VisLab commented Jul 11, 2024

Yes, it should be removed. If a dataset_description.json has a HedVersion field, it must be a valid HED version or it is considered to be an error. Thx!

@rwblair rwblair merged commit 9e622bd into bids-standard:master Jul 11, 2024
31 of 33 checks passed
happy5214 added a commit to happy5214/bids-validator that referenced this pull request Jul 11, 2024
rwblair added a commit that referenced this pull request Jul 12, 2024
@happy5214 happy5214 deleted the rewrite-hed branch July 16, 2024 04:02
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