Repository: mne-tools/mne-bids
Pull request #1240 (2024-03-20 14:24:21)

account for optional Z in time strs
<!--
Thanks for contributing this pull request (PR).
If this is your first time, make sure to read
[CONTRIBUTING.md](https://github.com/mne-tools/mne-bids/blob/main/CONTRIBUTING.md)
-->

PR Description
--------------

fix a problem that was reported here: https://mne.discourse.group/t/how-to-import-fnirs-data-from-a-local-dataset/8481/3?u=sappelhoff

cc @hoechenberger 

Merge checklist
---------------

Maintainer, please confirm the following before merging.
If applicable:

- [ ] All comments are resolved
- [ ] This is not your own PR
- [ ] All CIs are happy
- [ ] PR title starts with [MRG]
- [ ] [whats_new.rst](https://github.com/mne-tools/mne-bids/blob/main/doc/whats_new.rst) is updated
- [ ] New contributors have been added to [CITATION.cff](https://github.com/mne-tools/mne-bids/blob/main/CITATION.cff)
- [ ] PR description includes phrase "closes <#issue-number>"


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1240?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools) Report
Attention: Patch coverage is `50.00000%` with `1 lines` in your changes are missing coverage. Please review.
> Project coverage is 97.58%. Comparing base [(`87eea28`)](https://app.codecov.io/gh/mne-tools/mne-bids/commit/87eea28331ee09a2a333b6bf73d93d7e9a88a651?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools) to head [(`e1dd99e`)](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1240?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools).
> Report is 15 commits behind head on main.

| [Files](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1240?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools) | Patch % | Lines |
|---|---|---|
| [mne\_bids/read.py](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1240?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools#diff-bW5lX2JpZHMvcmVhZC5weQ==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1240?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main    #1240      +/-   ##
==========================================
- Coverage   97.61%   97.58%   -0.03%     
==========================================
  Files          40       40              
  Lines        8685     8665      -20     
==========================================
- Hits         8478     8456      -22     
- Misses        207      209       +2     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/mne-tools/mne-bids/pull/1240?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mne-tools).

hoechenberger: @sappelhoff I tried to give it a shot

Code isn't super pretty but I think the logical flow is very clear

Didn't run or write any tests yet
hoechenberger: I added some tests
sappelhoff: Very kind of you to include me in the commit message 🤣 

Thanks, I think this is much saner behavior now!!
hoechenberger: > Very kind of you to include me in the ~commit~ changelog message 🤣

yeah now worries, you started the PR after all :)

> Thanks, I think this is much saner behavior now!!

let's hope I / we didn't overlook something here … but I think in any case, we're behaving more correctly now than before!


Patch:
diff --git a/doc/whats_new.rst b/doc/whats_new.rst
index 0a13f031..66eea1fc 100644
--- a/doc/whats_new.rst
+++ b/doc/whats_new.rst
@@ -62,6 +62,8 @@ Detailed list of changes
   ``pandas.Int64Dtype`` instead of ``float64``, by `Eric Larson`_ (:gh:`1227`)
 - The :func:`mne_bids.copyfiles.copyfile_ctf` now accounts for files with ``.{integer}_meg4`` extension, instead of only .meg4,
   when renaming the files of a .ds folder, by `Mara Wolter`_ (:gh:`1230`)
+- We fixed handling of time zones when reading ``*_scans.tsv`` files; specifically, non-UTC timestamps are now processed correctly,
+  by `Stefan Appelhoff`_ and `Richard Höchenberger`_  (:gh:`1240`)
 
 ⚕️ Code health
 ^^^^^^^^^^^^^^
diff --git a/mne_bids/read.py b/mne_bids/read.py
index 365cdd22..375b427c 100644
--- a/mne_bids/read.py
+++ b/mne_bids/read.py
@@ -327,12 +327,32 @@ def _handle_scans_reading(scans_fname, raw, bids_path):
     # extract the acquisition time from scans file
     acq_time = acq_times[row_ind]
     if acq_time != "n/a":
-        # microseconds in the acquisition time is optional
+        # BIDS allows the time to be stored in UTC with a zero time-zone offset, as
+        # indicated by a trailing "Z" in the datetime string. If the "Z" is missing, the
+        # time is represented as "local" time. We have no way to know what the local
+        # time zone is at the *acquisition* site; so we simply assume the same time zone
+        # as the user's current system (this is what the spec demands anyway).
+        acq_time_is_utc = acq_time.endswith("Z")
+
+        # microseconds part in the acquisition time is optional; add it if missing
         if "." not in acq_time:
-            # acquisition time ends with '.%fZ' microseconds string
-            acq_time += ".0Z"
-        acq_time = datetime.strptime(acq_time, "%Y-%m-%dT%H:%M:%S.%fZ")
-        acq_time = acq_time.replace(tzinfo=timezone.utc)
+            if acq_time_is_utc:
+                acq_time = acq_time.replace("Z", ".0Z")
+            else:
+                acq_time += ".0"
+
+        date_format = "%Y-%m-%dT%H:%M:%S.%f"
+        if acq_time_is_utc:
+            date_format += "Z"
+
+        acq_time = datetime.strptime(acq_time, date_format)
+
+        if acq_time_is_utc:
+            # Enforce setting timezone to UTC without additonal conversion
+            acq_time = acq_time.replace(tzinfo=timezone.utc)
+        else:
+            # Convert time offset to UTC
+            acq_time = acq_time.astimezone(timezone.utc)
 
         logger.debug(
             f"Loaded {scans_fname} scans file to set " f"acq_time as {acq_time}."


Test patch:
diff --git a/mne_bids/tests/test_read.py b/mne_bids/tests/test_read.py
index ca83298a..7d489ca3 100644
--- a/mne_bids/tests/test_read.py
+++ b/mne_bids/tests/test_read.py
@@ -601,7 +601,7 @@ def test_handle_scans_reading(tmp_path):
     acq_time_str = scans_tsv["acq_time"][0]
     acq_time = datetime.strptime(acq_time_str, "%Y-%m-%dT%H:%M:%S.%fZ")
     acq_time = acq_time.replace(tzinfo=timezone.utc)
-    new_acq_time = acq_time_str.split(".")[0]
+    new_acq_time = acq_time_str.split(".")[0] + "Z"
     assert acq_time == raw_01.info["meas_date"]
     scans_tsv["acq_time"][0] = new_acq_time
     _to_tsv(scans_tsv, scans_path)
@@ -609,12 +609,30 @@ def test_handle_scans_reading(tmp_path):
     # now re-load the data and it should be different
     # from the original date and the same as the newly altered date
     raw_02 = read_raw_bids(bids_path)
-    new_acq_time += ".0Z"
+    new_acq_time = new_acq_time.replace("Z", ".0Z")
     new_acq_time = datetime.strptime(new_acq_time, "%Y-%m-%dT%H:%M:%S.%fZ")
     new_acq_time = new_acq_time.replace(tzinfo=timezone.utc)
     assert raw_02.info["meas_date"] == new_acq_time
     assert new_acq_time != raw_01.info["meas_date"]
 
+    # Test without optional zero-offset UTC time-zone indicator (i.e., without trailing
+    # "Z")
+    for has_microsecs in (True, False):
+        new_acq_time_str = "2002-12-03T19:01:10"
+        date_format = "%Y-%m-%dT%H:%M:%S"
+        if has_microsecs:
+            new_acq_time_str += ".0"
+            date_format += ".%f"
+
+        scans_tsv["acq_time"][0] = new_acq_time_str
+        _to_tsv(scans_tsv, scans_path)
+
+        # now re-load the data and it should be different
+        # from the original date and the same as the newly altered date
+        raw_03 = read_raw_bids(bids_path)
+        new_acq_time = datetime.strptime(new_acq_time_str, date_format)
+        assert raw_03.info["meas_date"] == new_acq_time.astimezone(timezone.utc)
+
 
 @pytest.mark.filterwarnings(warning_str["channel_unit_changed"])
 def test_handle_scans_reading_brainvision(tmp_path):