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

[ENH] Extend date time information to include optional UTC syntax, warn about FIF requirements #546

Merged
merged 10 commits into from
Aug 8, 2020

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Jul 27, 2020

In https://github.com/bids-standard/bids-validator/pull/1019 I wanted to add the validation of the newly introduced "fractional seconds" that we allow for AcquisitionTime in scans.tsv files.

While discussing that code, we stumbled over potential incongruencies between RFC3339 and the way that standard is implemented in BIDS.

In this PR we may clarify/fix some aspects ... or in the least we'll add a link and fix a typo (see
de4ab9f )

EDIT: we are now adding optional Z syntax to indicate UTC time in accordance with RFC3339

  • adjust validator for Z syntax

@sappelhoff
Copy link
Member Author

I now also link to the Units section from the modality specific MEG, EEG, and iEEG sections to

  1. avoid duplicating explanations
  2. drop the unspecific reference to ISO8601 (we are using a specific subset of this standard)

while doing that, I found the following sentence:

It does not need to be fully detailed, depending on local REB/IRB ethics board policy.

repeated across the files.

For now I dropped it from the three files and added it to the Units section. However, I think we can drop this sentence completely, because it is more or less covered in the point on anonymization that we have.

@sappelhoff sappelhoff changed the title [MISC] add link for rfc3339, fix typo [FIX] clarify date time information: rfc3339 Jul 27, 2020
@sappelhoff sappelhoff changed the title [FIX] clarify date time information: rfc3339 FIX: clarify date time information: rfc3339 Aug 2, 2020
@sappelhoff sappelhoff changed the title FIX: clarify date time information: rfc3339 FIX: clarify date time information: rfc3339, warn about FIF requirements Aug 2, 2020
src/02-common-principles.md Outdated Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

Just some minor edits, but it looks good to me.

@sappelhoff sappelhoff changed the title FIX: clarify date time information: rfc3339, warn about FIF requirements ENH: Extend date time information to include optional UTC syntax, warn about FIF requirements Aug 3, 2020
@sappelhoff sappelhoff added the enhancement New feature or request label Aug 3, 2020
Copy link
Collaborator

@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.

Two very minor suggestions.

src/02-common-principles.md Outdated Show resolved Hide resolved
src/02-common-principles.md Outdated Show resolved Hide resolved
Co-authored-by: Chris Markiewicz <[email protected]>
Copy link
Member

@tsalo tsalo 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
Collaborator

@franklin-feingold franklin-feingold left a comment

Choose a reason for hiding this comment

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

lgtm thanks @sappelhoff

@sappelhoff sappelhoff changed the title ENH: Extend date time information to include optional UTC syntax, warn about FIF requirements [ENH] Extend date time information to include optional UTC syntax, warn about FIF requirements Aug 8, 2020
@sappelhoff sappelhoff merged commit 565fde3 into bids-standard:master Aug 8, 2020
@sappelhoff sappelhoff deleted the dt branch August 8, 2020 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants