[ENH] Retain sub-second resolution in scans files by tsalo · Pull Request #451 · nipy/heudiconv

tsalo · 2020-05-12T17:40:48Z

Closes #447.

Changes proposed:

Keep sub-second timing information in ContentTime when compiling acq_time for scans.tsv files.

yarikoptic · 2020-05-12T19:05:39Z

heudiconv/bids.py

-        acq_time = datetime.strptime(td, '%H%M%S%Y%m%d').isoformat()
+        time = dcm_data.ContentTime
+        td = time + ':' + date
+        acq_time = datetime.strptime(td, '%H%M%S.%f:%Y%m%d').isoformat()


is .%f guaranteed to be present (above code would work without)? I wonder if there should be a check for '.' to be found in td?
If you could while at it place it into an auxiliary function and give it a dedicated tiny unittest - would be awesome... but if not -- that is ok ;) just feels like worthwhile

Sounds good. Just added a new function in utils, as well as a test for it.

codecov · 2020-05-12T19:17:01Z

Codecov Report

Merging #451 into master will increase coverage by 1.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
+ Coverage   74.99%   76.14%   +1.14%     
==========================================
  Files          35       36       +1     
  Lines        2863     2959      +96     
==========================================
+ Hits         2147     2253     +106     
+ Misses        716      706      -10

Impacted Files	Coverage Δ
heudiconv/tests/test_heuristics.py	`100.00% <ø> (ø)`
heudiconv/bids.py	`82.97% <100.00%> (ø)`
heudiconv/tests/test_main.py	`100.00% <100.00%> (ø)`
heudiconv/tests/test_utils.py	`100.00% <100.00%> (ø)`
heudiconv/utils.py	`90.45% <100.00%> (+0.37%)`	⬆️
heudiconv/tests/test_convert.py	`100.00% <0.00%> (ø)`
heudiconv/convert.py	`84.90% <0.00%> (+5.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75f2850...c5fe64f. Read the comment docs.

Made it also .000000 just for the sake of making robust since %f expects microseconds

yarikoptic · 2020-05-13T00:51:12Z

pushed a little tune up and then decided to check BIDS... heh heh

https://bids-specification.readthedocs.io/en/stable/03-modality-agnostic-files.html#scans-file

Datetime should be expressed in the following format 2009-06-15T13:45:30 (year, month, day, hour 
(24h), minute, second; this is equivalent to the RFC3339 "date-time" format, time zone is always 
assumed as local time).

and according to https://tools.ietf.org/html/rfc3339 and https://en.wikipedia.org/wiki/ISO_8601 seems sub-second is allowed (phew) BUT also wikipedia says However, the number of decimal places needs to be agreed to by the communicating parties. For example, in Microsoft SQL Server, the precision of a decimal fraction is 3, i.e., "yyyy-mm-ddThh:mm:ss[.mmm]".

So I am afraid that

we need to submit a PR to bids-specification suggesting allowing .[mmmmmm] before we could change this column formatting. (takers?)
but meanwhile we could just add a new one, e.g. acq_time_precise or alike. (I will push a commit or two for that here now while at it)

…rasp in such tests IMHO

…seconds

yarikoptic · 2020-05-13T01:28:20Z

I am not yet sure if I like it but here it was pushed. @tsalo could you please raise a question or a PR with bids-specification about .microseconds? ;)

tsalo · 2020-05-13T03:02:14Z

@yarikoptic My bad, I should have reviewed the specification before opening the issue/PR. I've opened bids-standard/bids-specification#469, so we'll see if we can get microseconds supported.

That allows to remove code duplication of header column names

yarikoptic · 2020-05-13T21:04:27Z

Since BIDS community is so receptive to the change, may be we could just proceed with subsecond in acq_time (and later update BIDSVersion we claim to be compatible with whenever that gets released) ! Meanwhile I had paid tribute to my laziness in the last commit I did last night -- and split it now into two, so we could just drop the last one which introduces acq_time_precise if we decide to go that route ;)

yarikoptic · 2020-05-19T18:12:08Z

So, @tsalo -- should I just drop the last commit and let's proceed with new improved acq_time? ;-)

tsalo · 2020-05-19T18:16:03Z

@yarikoptic That sounds good to me!

yarikoptic · 2020-05-19T22:05:56Z

ok, done. For the record -- pushed _abandoned_acq_time_precise tag with that abandoned now commit into our fork on https://github.com/dbic/heudiconv

tsalo · 2020-05-27T20:03:05Z

Now that bids-standard/bids-specification#470 is merged, can we merge this or should we wait for the validator (https://github.com/bids-standard/bids-validator/issues/957)?

yarikoptic · 2020-05-28T03:00:55Z

oh, we definitely should merge ;)

Various improvements and compatibility/support (dcm2niix, datalad, duecredit) changes. Major change is placement of output files to the target output directory during conversion. - #454 zenodo referencing in README.rst and support for ducredit for heudiconv and reproin heuristic - #445 more tutorial references in README.md - [#485][] placed files during conversion right away into the target directory (with a `_heudiconv???` suffix, renamed into ultimate target name later on), which avoids hitting file size limits of /tmp ([#481][]) and helped to avoid a regression in dcm2nixx 1.0.20201102 - #477 replaced `rec-<magnitude|phase>` with `part-<mag|phase>` now that BIDS supports the part entity - #473 made default for CogAtlasID to be a TODO URL - #459 made AcquisitionTime used for acq_time scans file field - #451 retained sub-second resolution in scans files - #442 refactored code so there is now heudiconv.main.workflow for more convenient use as a Python module - minimal version of nipype set to 1.2.3 to guarantee correct handling of DWI files ([#480][]) - `heudiconvDCM*` temporary directories are removed now ([#462][]) - compatibility with DataLad 0.13 ([#464][]) - #443 pathlib as a dependency (we are Python3 only now) * tag 'v0.9.0': Add a helper rule to upload to pypi update changelog reference as part of prep release [DATALAD RUNCMD] prepare the release CHANGELOG entry for 0.9.0

Various improvements and compatibility/support (dcm2niix, datalad, duecredit) changes. Major change is placement of output files to the target output directory during conversion. - #454 zenodo referencing in README.rst and support for ducredit for heudiconv and reproin heuristic - #445 more tutorial references in README.md - [#485][] placed files during conversion right away into the target directory (with a `_heudiconv???` suffix, renamed into ultimate target name later on), which avoids hitting file size limits of /tmp ([#481][]) and helped to avoid a regression in dcm2nixx 1.0.20201102 - #477 replaced `rec-<magnitude|phase>` with `part-<mag|phase>` now that BIDS supports the part entity - #473 made default for CogAtlasID to be a TODO URL - #459 made AcquisitionTime used for acq_time scans file field - #451 retained sub-second resolution in scans files - #442 refactored code so there is now heudiconv.main.workflow for more convenient use as a Python module - minimal version of nipype set to 1.2.3 to guarantee correct handling of DWI files ([#480][]) - `heudiconvDCM*` temporary directories are removed now ([#462][]) - compatibility with DataLad 0.13 ([#464][]) - #443 pathlib as a dependency (we are Python3 only now) * tag 'v0.9.0': Do no bother ensuring that version changed - should be no changes

tsalo added 2 commits May 12, 2020 13:38

Retain sub-second resolution in scans files.

0f62a72

Fix tests.

5d1d4c6

yarikoptic reviewed May 12, 2020

View reviewed changes

Fix test again.

609aa27

tsalo and others added 3 commits May 12, 2020 16:26

Move datetime to new util function and add test.

4c25a8b

Fix test.

09a9ea2

ENH: Adjust to reflect it is microseconds that are parsed/reported

b95f67b

Made it also .000000 just for the sake of making robust since %f expects microseconds

yarikoptic added 2 commits May 12, 2020 20:59

RF(TST): remove one-time use variables - oneliner is much easier to g…

a1e5b33

…rasp in such tests IMHO

ENH: make get_datetime accept microseconds kwarg to not provide micro…

d3e247c

…seconds

tsalo mentioned this pull request May 13, 2020

Allow microseconds in scans file datetimes bids-standard/bids-specification#469

Closed

tsalo mentioned this pull request May 13, 2020

Allow fractional seconds in scans file datetimes bids-standard/legacy-validator#957

Closed

RF: centralize definition of columns in the _scans files

c5fe64f

That allows to remove code duplication of header column names

yarikoptic force-pushed the enh/retain-subsecond branch from d56ecf0 to fbff77e Compare May 13, 2020 21:02

yarikoptic force-pushed the enh/retain-subsecond branch from fbff77e to c5fe64f Compare May 19, 2020 22:04

yarikoptic merged commit ec53e59 into nipy:master May 28, 2020

tsalo deleted the enh/retain-subsecond branch October 26, 2020 18:04

yarikoptic added this to the 0.9.0 milestone Dec 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Retain sub-second resolution in scans files#451

[ENH] Retain sub-second resolution in scans files#451
yarikoptic merged 9 commits intonipy:masterfrom
tsalo:enh/retain-subsecond

tsalo commented May 12, 2020

Uh oh!

yarikoptic May 12, 2020

Uh oh!

tsalo May 12, 2020

Uh oh!

codecov bot commented May 12, 2020 •

edited

Loading

Uh oh!

yarikoptic commented May 13, 2020

Uh oh!

yarikoptic commented May 13, 2020

Uh oh!

tsalo commented May 13, 2020

Uh oh!

yarikoptic commented May 13, 2020

Uh oh!

yarikoptic commented May 19, 2020

Uh oh!

tsalo commented May 19, 2020

Uh oh!

yarikoptic commented May 19, 2020

Uh oh!

tsalo commented May 27, 2020

Uh oh!

yarikoptic commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tsalo commented May 12, 2020

Uh oh!

yarikoptic May 12, 2020

Choose a reason for hiding this comment

Uh oh!

tsalo May 12, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yarikoptic commented May 13, 2020

Uh oh!

yarikoptic commented May 13, 2020

Uh oh!

tsalo commented May 13, 2020

Uh oh!

yarikoptic commented May 13, 2020

Uh oh!

yarikoptic commented May 19, 2020

Uh oh!

tsalo commented May 19, 2020

Uh oh!

yarikoptic commented May 19, 2020

Uh oh!

tsalo commented May 27, 2020

Uh oh!

yarikoptic commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented May 12, 2020 •

edited

Loading