[ENH] Allow fractional seconds in scans file datetimes #470
Conversation
There was a problem hiding this comment.
Thanks @tsalo. Could you please verify the state of the bids-validator regarding your changes?
- does it validate the RFC3339 format in
scans.tsvat all? - does it throw an error if we now optionally include microseconds?
answering these questions will help us to see what we need to implement in the validator before merging this PR (or in parallel to working on this PR)
|
@sappelhoff It look like bids-validator does validate the scans file, and it should raise an error (number 84, specifically) if microseconds are included. See here and here. I can edit list.js easily enough, since the part that needs to be changed is essentially just a json. However, I don't know JavaScript, so I have no idea how to update checkAcqTimeFormat.js to allow optional sections in the datetime format that is being checked. |
src/03-modality-agnostic-files.md
Outdated
| decreased. Dates that are shifted for anonymization purposes should be set to a | ||
| year 1925 or earlier to clearly distinguish them from unmodified data. Shifting | ||
| dates is RECOMMENDED, but not required. | ||
| in the following format `2009-06-15T13:45:30[.000000]` (year, month, day, |
There was a problem hiding this comment.
Can we here or somewhere state that the number of fractional digits is up to the user?
There was a problem hiding this comment.
What do you think of "No specific precision is required for fractional seconds, but the precision should be consistent across the dataset"?
There was a problem hiding this comment.
No specific precision is required for fractional seconds, but the precision should be consistent across the dataset
👍 with a capital SHOULD
|
ok, thanks for checking @tsalo
In that case, opening an issue on bids-validator would be a good idea and hopefully we find somebody to help us. Worst case we'll have to dig in ourselves (we'll see). |
|
I just opened bids-standard/bids-validator#957. We'll see if anyone has the time and bandwidth to help. |
There was a problem hiding this comment.
Even though it might lead to some BIDS tools breaking
- addressing compatibility issue should be quite easy
- necessity and benefits of adding sub-second precision IMHO outweigh aforementioned compatibility issue
- waiting for BIDS 2.0 which has no clear roadmap/ETA to introduce such a change would be overkill
src/03-modality-agnostic-files.md
Outdated
| person based on the date and time of their scan would be decreased. Dates that | ||
| are shifted for anonymization purposes should be set to a year 1925 or earlier | ||
| to clearly distinguish them from unmodified data. Shifting dates is | ||
| RECOMMENDED, but not required. |
There was a problem hiding this comment.
Can I be annoying and ask that you move each new sentence to a new line? This is a good example of a small change that forces the entire paragraph to be in the diff because we don't break at sentence boundaries. If we're going to have a big diff, why not make it one that helps us have smaller diffs in the future?
|
Should note I agree with @yarikoptic. The spec doesn't have to wait on the validator. The validator is obliged to follow the spec. |
|
cool, we have 4 approvals (although they were dismissed by the last commit), so I am merging now. thanks @tsalo |
* upstream/master: [DOC] Auto-generate changelog entry for PR bids-standard#470 Clarify precision of fractional seconds. Separate sentences. Square brackets seem to be a problem. Wrangle line lengths. Add optional fractional seconds to scans file datetimes.
Closes #469. Explicitly allows fractional seconds (i.e., sub-second resolution) in
acq_timecolumn of Scans file. Fractional seconds are (1) optional, (2) must follow a period, and (3) have no restriction on precision.