Allow filling of acq_time when AcquisitionDate AcquisitionTime missing #614

psadil · 2022-12-09T18:46:15Z

Closes #612

psadil · 2023-01-22T20:25:03Z

Following up on this. Does this patch look viable?

yarikoptic

Sorry it took so long!

not yet sure on UID hash - left comment/idea, sorry didn't dig in detail yet
would need python 3.7 incompatibility (:=) addressed
docstrings should follow numpy
push and hopefully we get CI running on it

yarikoptic · 2023-02-08T22:33:39Z

heudiconv/dicoms.py

-    dicom_time_str = dcm_date + dcm_time.split('.', 1)[0]  # YYYYMMDDHHMMSS
-    # convert to epoch
-    return calendar.timegm(time.strptime(dicom_time_str, '%Y%m%d%H%M%S'))
+    if (dicom_datetime := get_datetime_from_dcm(dicom)):


:= was added in python 3.8. We still remain compatible with 3.7.

Suggested change

if (dicom_datetime := get_datetime_from_dcm(dicom)):

dicom_datetime = get_datetime_from_dcm(dicom)

if dicom_datetime:

The walrus operator has now been removed

rg ":=" utils/test-compare-two-versions.sh 16:outdir=${OUTDIR:=compare-versions}

heudiconv/dicoms.py

yarikoptic · 2023-02-08T22:38:07Z

heudiconv/dicoms.py

+    Returns:
+        int: either an int representing when the first dicom was created, 
+            in number of seconds since epoch, or if no datetime info is found then a hash of the
+            SeriesInstanceUID (meaningless value, but reproducible)


if a "hash" -- it would not be properly sortable between different dicom series. I can't recall ATM if that might not make it a trouble somewhere - needs checking on how datetime is used -- if not used anywhere for sorting.

heudiconv/dicoms.py

yarikoptic · 2023-02-08T22:41:27Z

heudiconv/dicoms.py

+        dcm_data (pydicom.FileDataset): dicom with header, e.g., as ready by pydicom.dcmread
+
+    Returns:
+        datetime.datetime | None: one of several datetimes that are related to when the scan occurred. 


please make docstrings follow numpy docstring format https://numpydoc.readthedocs.io/en/latest/format.html#parameters

heudiconv/dicoms.py

yarikoptic · 2023-02-08T22:48:12Z

heudiconv/tests/test_dicoms.py

+def test_get_datetime_from_dcm():
+    import datetime
+    typical_dcm = dcm.dcmread(op.join(TESTS_DATA_PATH, 'phantom.dcm'), stop_before_pixels=True)
+    XA30_enhanced_dcm = dcm.dcmread(op.join(TESTS_DATA_PATH, 'MRI_102TD_PHA_S.MR.Chen_Matthews_1.3.1.2022.11.16.15.50.20.357.31204541.dcm'), stop_before_pixels=True)


I don't know what is our widest line , but this might be aiming to become a winner! Could we keep them within e.g. 120 chars?

psadil · 2023-02-10T22:03:10Z

Thanks! I will take a look at this in the next few days

psadil · 2023-02-10T22:08:15Z

Just in case: is there a style or contributing guide that I'm not seeing? Something the specifies, e.g., docstring format and line length

yarikoptic · 2023-02-14T03:51:54Z

Just in case: is there a style or contributing guide that I'm not seeing? Something the specifies, e.g., docstring format and line length

indeed there is none yet unfortunately, so everything "rests" on uniformity with "prior art". We should formalize some , filed #636 , contributions on that end are welcome - but may be we would push on it this wed...

As for docstrings we have only

❯ git grep __docformat
heudiconv/bids.py:__docformat__ = "numpy"
heudiconv/convert.py:__docformat__ = "numpy"

;-)

psadil · 2023-02-14T17:28:55Z

❯ git grep __docformat
heudiconv/bids.py:__docformat__ = "numpy"
heudiconv/convert.py:__docformat__ = "numpy"

;-)

Thanks for pointing that out! Still learning and I didn't think to look for something like that. :)

yarikoptic · 2023-03-07T02:07:19Z

@psadil are you planing to continue working on this PR? There is a number of needed changes for compatibility etc, besides discussion of sorting even.

psadil · 2023-03-07T03:11:12Z

@psadil are you planing to continue working on this PR? There is a number of needed changes for compatibility etc, besides discussion of sorting even.

Oh, sorry for the miscommunication. I am planning on continuing the pull request but was waiting on a reply from you to this comment. That is, with regards to setting mtime in a way that is both A) reproducible and B) allows sorting of the files based on when the images were collected, I'm not seeing any header field that would be required in every valid DICOM (only flavors of "optional" or "mandatory if known"). So,

Could reproducible mtime in any way be designated as a "feature" that relies on the existence of a particular set of fields in the DICOM header (specifically, fields that might not be present in every valid DICOM)?

yarikoptic · 2023-03-07T14:00:02Z

heudiconv/dicoms.py

-        # Reset the date to match the one from the dicom, not from the
-        # filesystem so we could sort reproducibly
+    def _assign_dicom_time(ti: tarfile.TarInfo) -> tarfile.TarInfo:
+        # Try to reset the date to match the one of the last commit, not from the


what commit? there is no assumption in heudiconv on having dataset in git here. And is this TODO?

That's a comment that was there when I forked initially. I see that since then you've updated the note. When updating this pull request I'll make sure to integrate the more recent commits/handle any merge conflicts

…ike a time

psadil · 2023-03-09T19:18:41Z

@yarikoptic, this is now ready for review again. Thanks!

yarikoptic · 2023-03-09T19:48:11Z

codespell has already smth to say

Error: ./heudiconv/dicoms.py:175: groupped ==> grouped
Error: ./heudiconv/tests/test_dicoms.py:151: useable ==> usable

codecov · 2023-03-09T19:52:04Z

Codecov Report

Patch coverage: 76.47% and project coverage change: -0.07 ⚠️

Comparison is base (71f4dbe) 81.36% compared to head (74e3fcc) 81.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #614      +/-   ##
==========================================
- Coverage   81.36%   81.30%   -0.07%     
==========================================
  Files          42       42              
  Lines        3837     3888      +51     
==========================================
+ Hits         3122     3161      +39     
- Misses        715      727      +12

Impacted Files	Coverage Δ
heudiconv/tests/test_dicoms.py	`85.88% <69.23%> (-14.12%)`	⬇️
heudiconv/dicoms.py	`85.11% <83.33%> (-0.66%)`	⬇️
heudiconv/bids.py	`90.51% <100.00%> (+0.63%)`	⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

psadil · 2023-03-09T20:21:05Z

codespell has already smth to say

🤦 Thanks. fixed now

psadil · 2023-03-16T21:47:17Z

Following up on this -- I fixed the typos edited by codespell. GitHub tells me that other changes have been requested, but I think those all relate to the initial draft of this request.

yarikoptic · 2023-03-17T12:51:15Z

looks good and CI is happy -- let's proceed. Thank you @psadil!

psadil · 2023-03-17T15:58:15Z

@yarikoptic thank you for walking through this!

github-actions · 2023-05-08T14:41:21Z

🚀 PR was released in v0.13.0 🚀

psadil added 2 commits December 9, 2022 13:42

reduce requirements for date and time fields in dicom headers

e7a2409

handle merge conflicts

4230d6a

yarikoptic requested changes Feb 8, 2023

View reviewed changes

yarikoptic mentioned this pull request Feb 14, 2023

provide CONTRIBUTING.md (or .rst) #636

Closed

yarikoptic mentioned this pull request Mar 7, 2023

IntendedFor population crashing heudiconv with Siemens XA dicoms #649

Closed

2 tasks

yarikoptic reviewed Mar 7, 2023

View reviewed changes

psadil added 2 commits March 9, 2023 11:54

Merge branch 'master' into fix/acqtime-for-xa

12edce8

Ensure that the extracted reproducible int corresponds to something l…

de5b2d1

…ike a time

codespell

74e3fcc

psadil mentioned this pull request Mar 9, 2023

Mention codespell in CONTRIBUTING.rst ? #650

Open

2 tasks

yarikoptic merged commit b2e5d03 into nipy:master Mar 17, 2023

yarikoptic added the minor Increment the minor version when merged label Mar 17, 2023

psadil deleted the fix/acqtime-for-xa branch March 17, 2023 15:58

github-actions bot added the released label May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow filling of acq_time when AcquisitionDate AcquisitionTime missing #614

Allow filling of acq_time when AcquisitionDate AcquisitionTime missing #614

psadil commented Dec 9, 2022

psadil commented Jan 22, 2023

yarikoptic left a comment

yarikoptic Feb 8, 2023

psadil Mar 9, 2023 •

edited

Loading

yarikoptic Feb 8, 2023

yarikoptic Feb 8, 2023

yarikoptic Feb 8, 2023

psadil commented Feb 10, 2023

psadil commented Feb 10, 2023

yarikoptic commented Feb 14, 2023 •

edited

Loading

psadil commented Feb 14, 2023

yarikoptic commented Mar 7, 2023

psadil commented Mar 7, 2023

yarikoptic Mar 7, 2023

psadil Mar 7, 2023

psadil commented Mar 9, 2023

yarikoptic commented Mar 9, 2023

codecov bot commented Mar 9, 2023 •

edited

Loading

psadil commented Mar 9, 2023 •

edited

Loading

psadil commented Mar 16, 2023

yarikoptic commented Mar 17, 2023

psadil commented Mar 17, 2023

github-actions bot commented May 8, 2023

	if (dicom_datetime := get_datetime_from_dcm(dicom)):
	dicom_datetime = get_datetime_from_dcm(dicom)
	if dicom_datetime:

Allow filling of acq_time when AcquisitionDate AcquisitionTime missing #614

Allow filling of acq_time when AcquisitionDate AcquisitionTime missing #614

Conversation

psadil commented Dec 9, 2022

psadil commented Jan 22, 2023

yarikoptic left a comment

Choose a reason for hiding this comment

yarikoptic Feb 8, 2023

Choose a reason for hiding this comment

psadil Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

yarikoptic Feb 8, 2023

Choose a reason for hiding this comment

yarikoptic Feb 8, 2023

Choose a reason for hiding this comment

yarikoptic Feb 8, 2023

Choose a reason for hiding this comment

psadil commented Feb 10, 2023

psadil commented Feb 10, 2023

yarikoptic commented Feb 14, 2023 • edited Loading

psadil commented Feb 14, 2023

yarikoptic commented Mar 7, 2023

psadil commented Mar 7, 2023

yarikoptic Mar 7, 2023

Choose a reason for hiding this comment

psadil Mar 7, 2023

Choose a reason for hiding this comment

psadil commented Mar 9, 2023

yarikoptic commented Mar 9, 2023

codecov bot commented Mar 9, 2023 • edited Loading

Codecov Report

psadil commented Mar 9, 2023 • edited Loading

psadil commented Mar 16, 2023

yarikoptic commented Mar 17, 2023

psadil commented Mar 17, 2023

github-actions bot commented May 8, 2023

psadil Mar 9, 2023 •

edited

Loading

yarikoptic commented Feb 14, 2023 •

edited

Loading

codecov bot commented Mar 9, 2023 •

edited

Loading

psadil commented Mar 9, 2023 •

edited

Loading