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] Update HED appendix to comply with current HED version #970

Merged
merged 26 commits into from
Jan 23, 2022
Merged

[FIX] Update HED appendix to comply with current HED version #970

merged 26 commits into from
Jan 23, 2022

Conversation

VisLab
Copy link
Member

@VisLab VisLab commented Jan 10, 2022

The HED.md appendix has been rewritten to use the current version of HED.

Please review:
@sappelhoff @dungscout96 @happy5214 @tpatpa @smakeig @dorahermes @IanCa @arnodelorme @Remi-Gau

@VisLab
Copy link
Member Author

VisLab commented Jan 10, 2022

I noticed that the link checker failed on the glossary section, which is not part of this pull request. Perhaps someone could address?

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! I like the revamp - it's more concise and clear. I left a couple of comments.

src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
src/99-appendices/03-hed.md Show resolved Hide resolved
src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
src/99-appendices/03-hed.md Outdated Show resolved Hide resolved
@sappelhoff
Copy link
Member

I noticed that the link checker failed on the glossary section, which is not part of this pull request. Perhaps someone could address?

@VisLab we have to ignore the LinkChecker for now, unfortunately (technical detail, see this comment and the six that follow: #892 (review) )

VisLab and others added 12 commits January 10, 2022 12:40
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

I had a quick look and this seems like a clearer and concise "intro" to HED.

Copy link
Member

@dorahermes dorahermes left a comment

Choose a reason for hiding this comment

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

Very clear!! I only had one small question on whether there are any previous BIDS examples that use the long form. If there are existing BIDS examples with the long form, it would make sense to have one example that uses both the long and short forms, while stating that the short form is recommended.

If there are no BIDS examples with the long form, this looks good to me!

src/99-appendices/03-hed.md Show resolved Hide resolved
Copy link
Member

@dorahermes dorahermes left a comment

Choose a reason for hiding this comment

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

lgtm

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.

@VisLab thanks for addressing most of my suggestions. A few were left open and I simply committed those that I deemed uncontroversial, for the diff see: 0cf0835

I can revert that commit if you disagree.

That leaves only one "controversial" point from my review open:

  • You often refer to "tools" that "do something" ... I made some suggestions to change this throughout the appendix to "HED specific tools" that "MUST do this and that" (... implicitly meaning that HED tool must behave in that way)
  • I find that language more clear: If you agree, feel free to accept these remaining suggestions, else I'd appreciate if you could suggest an alternative, so that we can more precisely talk about these "tools" (this was also a frequently raised point in recent HED paper reviews)

PS: if you missed my remaining suggestions because they were so many, you need to click on the "load more", or go to the files changed overview: https://github.com/bids-standard/bids-specification/pull/970/files

VisLab and others added 4 commits January 15, 2022 12:17
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>
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.

LGTM!

@sappelhoff sappelhoff added this to the 1.7.0 milestone Jan 15, 2022
@sappelhoff
Copy link
Member

Thanks for the update, Kay. And thanks for the reviews everyone else - I am merging this now.

@sappelhoff sappelhoff merged commit f888291 into bids-standard:master Jan 23, 2022
@VisLab VisLab deleted the hed_update branch February 9, 2022 12:57
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