[ENH] Add the ability of users to specify an explicit HED.xml schema for validation. #527

VisLab · 2020-07-10T16:14:42Z

closes #537

We have refactored the HED validation in BIDS so that it does not depend on a specific HED schema version. The HED schema itself is under active development by a hed-standard group. As this schema specification evolves, people will be tagging their data at a specific point in time. We do not want their tagging to become invalid because we changed the schema. They just need to specify the vocabulary they used.

This change will require some modifications of the BIDS validator. Our group is working on a draft pull request for those changes. When it is ready, we will do a regular request, but we would appreciate comments/suggestions during the process.

Corrected typo.

Changed HEDSchemaPath to be HEDSchemaFile in json sidecar

More carefully laid out what the preferred behavior is.

Fixed typeface and capitalization.

effigies · 2020-07-10T18:02:23Z

This seems like a good approach. I like that you can specify a version or an explicit schema. I can make more specific suggestions once this is marked ready for review, but here are some initial thoughts:

I would suggest HEDVersion instead of HEDSchemaVersion to be consistent with BIDSVersion.
Putting these values in dataset_description.json rather than _events.json would ensure that only one schema can be used within a dataset. I don't think we want to open up the possibility of multiple.
XML generally permits local or remote URIs for its schemata, so unless we have an explicit desire to disallow that, I would suggest permitting a URI. If a dataset wants to place it inside sourcedata, then it could use file://sourcedata/HED7.2.1.xml.

Also, does it make sense to have two separate keys HEDVersion (or HEDSchemaVersion) and HEDSchemaFile? It should be pretty easy to disambiguate a version string from a URI, if we want to combine, but I don't feel very strongly here.

VisLab · 2020-07-10T18:23:02Z

Agree with all suggestions....thanks.....

…ication

Moved the specification of which HED schema to validate against to the dataset_description.json file as suggested.

This optional field is used to specify the HED version or specific XML schema file used for HED validation.

Updated dataset_description to include HEDSchema. Fixed table spacing error this introduced.

effigies · 2020-07-15T21:20:20Z

src/03-modality-agnostic-files.md

@@ -27,6 +27,7 @@ Every dataset MUST include this file with the following fields:
 | EthicsApprovals    | OPTIONAL. List of ethics committee approvals of the research protocols and/or protocol identifiers.                                                                                                                                                                |
 | ReferencesAndLinks | OPTIONAL. List of references to publication that contain information on the dataset, or links.                                                                                                                                                                     |
 | DatasetDOI         | OPTIONAL. The Document Object Identifier of the dataset (not the corresponding paper).                                                                                                                                                                             |
+| HEDVersion         | OPTIONAL. HED version number or the name of HED XML schema file used to validate HED tags for study.                                                                                                                                              |


If there's a significant likelihood of backwards-incompatible changes, then this should probably be RECOMMENDED:

Suggested change

| HEDVersion | OPTIONAL. HED version number or the name of HED XML schema file used to validate HED tags for study. |

| HEDVersion | RECOMMENDED if HED tags are used. HED version number or the name of HED XML schema file used to validate HED tags for study. |

If it's mostly a way to test new features, then OPTIONAL seems fine.

VisLab · 2020-07-15T21:44:08Z

It should definitely be RECOMMENDED. The hed-standard group is doing a major redesign that I think will be a great improvement, both in terms of capability and usability. The schema will not be backwards compatible, though the validators should still work.

…

On Wed, Jul 15, 2020 at 4:20 PM Chris Markiewicz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/03-modality-agnostic-files.md <#527 (comment)> : > @@ -27,6 +27,7 @@ Every dataset MUST include this file with the following fields: | EthicsApprovals | OPTIONAL. List of ethics committee approvals of the research protocols and/or protocol identifiers. | | ReferencesAndLinks | OPTIONAL. List of references to publication that contain information on the dataset, or links. | | DatasetDOI | OPTIONAL. The Document Object Identifier of the dataset (not the corresponding paper). | +| HEDVersion | OPTIONAL. HED version number or the name of HED XML schema file used to validate HED tags for study. | If there's a significant likelihood of backwards-incompatible changes, then this should probably be RECOMMENDED: ⬇️ Suggested change -| HEDVersion | OPTIONAL. HED version number or the name of HED XML schema file used to validate HED tags for study. | +| HEDVersion | RECOMMENDED if HED tags are used. HED version number or the name of HED XML schema file used to validate HED tags for study. | If it's mostly a way to test new features, then OPTIONAL seems fine. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#527 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJCJOTXFXY72DYWH4WP4OTR3YMSJANCNFSM4OWXN3PA> .

Not sure whether this should be qualified by some statement that it is recommended if you are using HED tags to annotate.

happy5214 · 2020-07-17T15:28:37Z

This seems like a good approach. I like that you can specify a version or an explicit schema. I can make more specific suggestions once this is marked ready for review, but here are some initial thoughts:
* I would suggest `HEDVersion` instead of `HEDSchemaVersion` to be consistent with `BIDSVersion`.

* Putting these values in `dataset_description.json` rather than `_events.json` would ensure that only one schema can be used within a dataset. I don't think we want to open up the possibility of multiple.

* XML generally permits local or remote URIs for its schemata, so unless we have an explicit desire to disallow that, I would suggest permitting a URI. If a dataset wants to place it inside `sourcedata`, then it could use `file://sourcedata/HED7.2.1.xml`.
Also, does it make sense to have two separate keys HEDVersion (or HEDSchemaVersion) and HEDSchemaFile? It should be pretty easy to disambiguate a version string from a URI, if we want to combine, but I don't feel very strongly here.

Making a local path a URI would require the BIDS validator to parse it into a local path before passing it to the HED validator, as the latter does not accept URIs (including for remote schemata). Even if the HED validator were to accept URIs, it would not be able to find the file with the URI because the proposed URI is relative to the dataset directory rather than the execution directory, so the HED validator would have the wrong origin for the relative path. Keeping the filename under sourcedata/ as a simple name is much easier to implement.

I have implemented the other proposed changes (merging the two fields in _events.json into a single HEDVersion field in dataset_description.json). The pull request is at bids-standard/bids-validator#1015.

We don't need the URI syntax. The file will always be in sourcedata.

We aren't using URIs, just filenames in the sourcedata directory.

VisLab · 2020-08-02T12:21:01Z

@happy5214 and @VisLab would like to move forward with the merge. There will be additional suggestions/fixes but this merge is needed to move forward.

sappelhoff

Okay, fine with me to iterate on the examples, but then I suggest to make the brief intro sentences before the examples a bit more clear, so that readers know there is more to define in the JSONS, and that the examples are not "full examples".

sappelhoff · 2020-08-02T12:24:03Z

src/04-modality-specific-files/05-task-events.md

@@ -65,6 +65,28 @@ onset duration  trial_type  response_time stim_file
 5.6 0.6 stop  1.739 images/blue_square.jpg
 ```

+The accompanying JSON side car might be:


Suggested change

The accompanying JSON side car might be:

In the accompanying JSON sidecar, the `trial_type` column might look as follows:

src/04-modality-specific-files/05-task-events.md

sappelhoff · 2020-08-02T12:29:22Z

Finally, please have a look at the "Checks" section in the GitHub user interface of this Pull Request (see screenshot):

You currently have once check failing: Travis CI.

Please click on "Details" for more information

This will lead you to an overview, where you can click on "The build" to get information on WHY the check has failed

That will finally show you line by line what the outstanding issues are:

In your case, these are related to Markdown style. Please fix these issues as well, so that we can keep a consistent style throughout the BIDS specification. If you have issues or problems with that, just let me know and I can help you.

EDIT: in the failure report, the syntax like 66:1 means that for a given file there is an issue in line 66 at character 1

VisLab · 2020-08-02T12:46:12Z

I am doing the requested corrections.... but am having trouble figuring out what the spacing rules are for the description table for the able fences. Could you clarify for me?

…

On Sun, Aug 2, 2020 at 7:29 AM Stefan Appelhoff ***@***.***> wrote: Finally, please have a look at the "Checks" section in the GitHub user interface of this Pull Request (see screenshot): [image: image] <https://user-images.githubusercontent.com/9084751/89123002-425c8d00-d4cc-11ea-8ad1-f4d3592c41e1.png> You currently have once check failing: Travis CI. Please click on "Details" for more information [image: image] <https://user-images.githubusercontent.com/9084751/89123007-50aaa900-d4cc-11ea-965e-de02ce639253.png> This will lead you to an overview, where you can click on "The build" to get information on WHY the check has failed [image: image] <https://user-images.githubusercontent.com/9084751/89123013-6029f200-d4cc-11ea-8803-cb71916f5479.png> That will finally show you line by line what the outstanding issues are: [image: image] <https://user-images.githubusercontent.com/9084751/89123021-76d04900-d4cc-11ea-88cf-2017649acda9.png> In your case, these are related to Markdown style. Please fix these issues as well, so that we can keep a consistent style throughout the BIDS specification. If you have issues or problems with that, just let me know and I can help you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#527 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJCJOWLRHVLBXMXOP2BEILR6VL23ANCNFSM4OWXN3PA> .

sappelhoff · 2020-08-02T15:36:46Z

sure, a markdown table has fences made out of pipes | and dashes -

A good table looks like this:

| header1 | a very long header2 | header3          |
| ------- | ------------------- | ---------------- |
| val1    | val2                | a very long val3 |
| val4    | val5                | val6             |

a bad table (although technically correct syntax) looks like this (for example):

| header1 | a very long header2 | header3 |
| ------- | ------------------- | ------- |
| val1 | val2 | a very long val3 |
| val4 | val5 | val6 |

In a rendered state, both will look like this:

header1	a very long header2	header3
val1	val2	a very long val3
val4	val5	val6

VisLab · 2020-08-02T16:43:39Z

Okay, I have attempted to fix this.

sappelhoff · 2020-08-02T16:53:46Z

I see you fixed all errors, thanks! Now there remain two small outstanding issues (linked below) from my side and then a final review from @effigies (if he has time and energy):

[ENH] Add the ability of users to specify an explicit HED.xml schema for validation. #527 (comment)
[ENH] Add the ability of users to specify an explicit HED.xml schema for validation. #527 (comment)

Note, I think there was an correct duplication of text code. I removed it, but someone should check it.

effigies

Minor formatting and wording suggestions.

src/03-modality-agnostic-files.md

effigies · 2020-08-03T18:06:49Z

src/04-modality-specific-files/05-task-events.md

+hypothesis extensions of BIDS. Note that the `trial_type` and any additional
+columns in a TSV file SHOULD be documented in an accompanying JSON sidecar file.


Just adding a newline to keep future changes confined to fewer lines.

Suggested change

hypothesis extensions of BIDS. Note that the `trial_type` and any additional

columns in a TSV file SHOULD be documented in an accompanying JSON sidecar file.

hypothesis extensions of BIDS.

Note that the `trial_type` and any additional columns in a TSV file SHOULD be

documented in an accompanying JSON sidecar file.

Not sure if I got the correction right on this one. Were there supposed to be two spaces after BIDS.

src/04-modality-specific-files/05-task-events.md

effigies · 2020-08-04T17:52:03Z

src/99-appendices/03-hed.md

-relevant HED tags will then be associated with the event instance.
+
+The tags in the `HED` column of the `*_events.tsv` file are often specific to the individual event instances,
+while the common properties are represented by categorial values appearing in other columns. 


Suggested change

while the common properties are represented by categorial values appearing in other columns.

while the common properties are represented by categorical values appearing in other columns.

Corrected the spelling of categorical. Not sure about the removing of the space?

effigies · 2020-08-04T17:54:01Z

src/99-appendices/03-hed.md

+the categorical specifications, but should form the union before analysis. Further,
+the normal BIDS inheritance principle applies, so the data dictionaries can
+appear higher in the BIDS hierarchy.  


Suggested change

the categorical specifications, but should form the union before analysis. Further,

the normal BIDS inheritance principle applies, so the data dictionaries can

appear higher in the BIDS hierarchy.

the categorical specifications, but should form the union before analysis.

Further, the [inheritance principle](../02-common-principles.md#the-inheritance-principle) applies,

so the data dictionaries can appear higher in the BIDS hierarchy.

Corrected, but again, I am not sure about the rules that should be followed for inserting new lines and the extra spaces after periods.

Added a new line.

Corrected spelling of categorical.

Added the inheritance link.

VisLab · 2020-08-06T16:25:36Z

Is there anything else that needs attention on this?

sappelhoff · 2020-08-06T16:35:16Z

I think we can merge this now 👍

Just a note for you --> when a reviewer makes "code suggestions", you see these two buttons (screenshot):

You can use them to commit these suggestions directly to the code, which can often be easier and sometimes cleaner for the review process.

Regarding line breaks and spaces in the spec:

try to start a new line after each sentence
try to break very long lines at reasonable spots (up to you)
single spaces (no double spaces)
no trailing whitespace

sappelhoff · 2020-08-06T16:37:49Z

Thanks for your work and patience @VisLab @happy5214

VisLab · 2020-08-06T18:30:46Z

Thanks for your help. I'll know next time......

…

On Thu, Aug 6, 2020 at 11:38 AM Stefan Appelhoff ***@***.***> wrote: Thanks for your work and patience @VisLab <https://github.com/VisLab> @happy5214 <https://github.com/happy5214> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#527 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJCJOVZAVHXSW4LVYYK6MLR7LL6XANCNFSM4OWXN3PA> .

effigies · 2020-08-06T20:25:54Z

Thanks @VisLab! I appreciate your patience!

VisLab and others added 14 commits May 18, 2020 14:00

Minor typographic corrections

1665052

Added specification of HED schema to event.json sidecar

19587e1

Updated the wording of the HED schema string

13fa051

Add missing quote on key in last HED JSON example

aefb3b3

Fix link to HED spec and minor spelling and formatting fixes

47c5519

Added a HEDSchemaPath option to json sidecar

cf876dc

Added a HEDSchemaPath option

e3581f9

Added order of resolution in looking for HED schema

43a944e

Corrected a typo in the description

44a0bb3

Corrected another typo

05d0b53

Update 03-hed.md

d2b5810

Corrected typo.

Update 03-hed.md

06dc75b

Changed HEDSchemaPath to be HEDSchemaFile in json sidecar

Update 03-hed.md

a629a36

More carefully laid out what the preferred behavior is.

Update 03-hed.md

0f25c9a

Fixed typeface and capitalization.

happy5214 mentioned this pull request Jul 15, 2020

Allow datasets to specify HED versions or individual files as validation schema bids-standard/bids-validator#1015

Merged

VisLab added 7 commits July 15, 2020 13:24

Merge branch 'master' of https://github.com/bids-standard/bids-specif…

a8e4a81

…ication

Changed to HEDVersion in dataset_description.json

a3b134e

Moved the specification of which HED schema to validate against to the dataset_description.json file as suggested.

Added HEDVersion to the dataset description.

396ab21

This optional field is used to specify the HED version or specific XML schema file used for HED validation.

Merged previous changes

5ae795b

Updated specification for dataset_description.json

0f67cd9

Corrected spacing in dataset_description.json

62c9268

Updated dataset_description to include HEDSchema. Fixed table spacing error this introduced.

Corrected minor typo in the update

fc18b5c

effigies reviewed Jul 15, 2020

View reviewed changes

Changed OPTIONAL to RECOMMENDED for HEDVersion

c51607b

Not sure whether this should be qualified by some statement that it is recommended if you are using HED tags to annotate.

VisLab added 2 commits July 17, 2020 10:59

Fix the HEDVersion to just be a filename

827680e

We don't need the URI syntax. The file will always be in sourcedata.

Changed relative path to just XML filename

cd22bd5

We aren't using URIs, just filenames in the sourcedata directory.

Removed trailing white space in examples

9b4345e

sappelhoff reviewed Aug 2, 2020

View reviewed changes

VisLab added 3 commits August 2, 2020 07:46

Removed space before example

afb65e2

Removed extra line in a couple of places

bd410fa

Removed extra line before example

969ae45

VisLab added 2 commits August 2, 2020 11:29

Attempted to fix dataset_description table align; also a typo

2ad7c13

Travis wanted ending pipes, so they're back

8de2dc7

sappelhoff requested a review from effigies August 2, 2020 16:54

VisLab added 2 commits August 2, 2020 12:57

Clarified columns of events.tsv in json

a500452

Explained JSON sidecar values

340de69

Note, I think there was an correct duplication of text code. I removed it, but someone should check it.

sappelhoff approved these changes Aug 3, 2020

View reviewed changes

effigies reviewed Aug 4, 2020

View reviewed changes

VisLab added 6 commits August 4, 2020 13:18

Fixed spacing in HEDVersion table entry.

302e8cd

Update 05-task-events.md

a39764d

Added a new line.

Added another newline

79e3c8d

Update 03-hed.md

814e2e7

Corrected spelling of categorical.

Update 03-hed.md

43c8480

Added the inheritance link.

Two-space for hard line break

0445ad9

sappelhoff merged commit aa98a2d into bids-standard:master Aug 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Add the ability of users to specify an explicit HED.xml schema for validation. #527

[ENH] Add the ability of users to specify an explicit HED.xml schema for validation. #527

VisLab commented Jul 10, 2020 •

edited by sappelhoff

Loading

effigies commented Jul 10, 2020

VisLab commented Jul 10, 2020

effigies Jul 15, 2020

VisLab commented Jul 15, 2020 via email

happy5214 commented Jul 17, 2020

VisLab commented Aug 2, 2020

sappelhoff left a comment

sappelhoff Aug 2, 2020

sappelhoff commented Aug 2, 2020 •

edited

Loading

VisLab commented Aug 2, 2020 via email

sappelhoff commented Aug 2, 2020

VisLab commented Aug 2, 2020

sappelhoff commented Aug 2, 2020

effigies left a comment

effigies Aug 3, 2020

VisLab Aug 4, 2020

effigies Aug 4, 2020

VisLab Aug 4, 2020

effigies Aug 4, 2020

VisLab Aug 4, 2020

VisLab commented Aug 6, 2020

sappelhoff commented Aug 6, 2020

sappelhoff commented Aug 6, 2020

VisLab commented Aug 6, 2020 via email

effigies commented Aug 6, 2020

	\| HEDVersion \| OPTIONAL. HED version number or the name of HED XML schema file used to validate HED tags for study. \|
	\| HEDVersion \| RECOMMENDED if HED tags are used. HED version number or the name of HED XML schema file used to validate HED tags for study. \|

	The accompanying JSON side car might be:
	In the accompanying JSON sidecar, the `trial_type` column might look as follows:

		hypothesis extensions of BIDS. Note that the `trial_type` and any additional
		columns in a TSV file SHOULD be documented in an accompanying JSON sidecar file.

	while the common properties are represented by categorial values appearing in other columns.
	while the common properties are represented by categorical values appearing in other columns.

[ENH] Add the ability of users to specify an explicit HED.xml schema for validation. #527

[ENH] Add the ability of users to specify an explicit HED.xml schema for validation. #527

Conversation

VisLab commented Jul 10, 2020 • edited by sappelhoff Loading

effigies commented Jul 10, 2020

VisLab commented Jul 10, 2020

Choose a reason for hiding this comment

VisLab commented Jul 15, 2020 via email

happy5214 commented Jul 17, 2020

VisLab commented Aug 2, 2020

sappelhoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sappelhoff commented Aug 2, 2020 • edited Loading

VisLab commented Aug 2, 2020 via email

sappelhoff commented Aug 2, 2020

VisLab commented Aug 2, 2020

sappelhoff commented Aug 2, 2020

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VisLab commented Aug 6, 2020

sappelhoff commented Aug 6, 2020

sappelhoff commented Aug 6, 2020

VisLab commented Aug 6, 2020 via email

effigies commented Aug 6, 2020

VisLab commented Jul 10, 2020 •

edited by sappelhoff

Loading

sappelhoff commented Aug 2, 2020 •

edited

Loading