mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.72k stars 1.32k forks source link

Cannot set non-UTC timezone #9953

Open heilerich opened 3 years ago

heilerich commented 3 years ago

Describe the bug

There seems to be no obvious way to set utc_offset in the Info object. Setting it directly prints a warning and threatens with an error after the next release. The function set_meas_date only accepts UTC datetime objects.

Steps to reproduce

>>> import mne
>>> mne.__version__
'0.24.0'
>>> info = mne.create_info([''], 1)
>>> info['utc_offset'] = '+02:00'
<stdin>:1: DeprecationWarning: utc_offset cannot be set directly. This warning will turn into an error after 0.24
>>> recording = mne.io.RawArray([[]],info)
Creating RawArray with float64 data, n_channels=1, n_times=0
    Range : 0 ... -1 =      0.000 ...    -1.000 secs
Ready.
>>> from datetime import datetime, timezone, timedelta
>>> date = datetime.now().replace(tzinfo=timezone(timedelta(hours=2), 'Europe/Berlin'))
>>> recording.set_meas_date(date)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/conda/lib/python3.8/site-packages/mne/channels/channels.py", line 654, in set_meas_date
    meas_date = _handle_meas_date(meas_date)
  File "/opt/conda/lib/python3.8/site-packages/mne/annotations.py", line 690, in _handle_meas_date
    _check_dt(meas_date)  # run checks
  File "/opt/conda/lib/python3.8/site-packages/mne/utils/numerics.py", line 1029, in _check_dt
    raise ValueError('Date must be datetime object in UTC: %r' % (dt,))
ValueError: Date must be datetime object in UTC: datetime.datetime(2021, 11, 4, 11, 50, 49, 834556, tzinfo=datetime.timezone(datetime.timedelta(seconds=7200), 'Europe/Berlin'))

Expected results

Either utc_offset should be writable or set_meas_date should accept non-UTC datetime objects and set meas_date (after conversion) and utc_offset accordingly.

Actual results

See above.

Additional information

`mne.sys_info()` ``` Platform: Linux-5.10.75-flatcar-x86_64-with-glibc2.10 Python: 3.8.10 | packaged by conda-forge | (default, May 11 2021, 07:01:05) [GCC 9.3.0] Executable: /opt/conda/bin/python CPU: x86_64: 32 cores Memory: Unavailable (requires "psutil" package) mne: 0.24.0 numpy: 1.21.0 {blas=openblas, lapack=openblas} scipy: 1.7.1 matplotlib: 3.4.3 {backend=agg} sklearn: 1.0.1 numba: Not found nibabel: Not found nilearn: Not found dipy: Not found cupy: Not found pandas: 1.3.4 mayavi: Not found pyvista: Not found pyvistaqt: Not found ipyvtklink: Not found vtk: Not found PyQt5: Not found ipympl: Not found mne_qt_browser: Not found ```
welcome[bot] commented 3 years ago

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴🏽‍♂️

larsoner commented 3 years ago

I think you just need to add a .astimezone(utc), like:

>>> date = datetime.now().replace(tzinfo=timezone(timedelta(hours=2), 'Europe/Berlin'))
>>> date
datetime.datetime(2021, 11, 4, 9, 47, 12, 306954, tzinfo=datetime.timezone(datetime.timedelta(seconds=7200), 'Europe/Berlin'))
>>> date.astimezone(timezone.utc)
datetime.datetime(2021, 11, 4, 7, 47, 12, 306954, tzinfo=datetime.timezone.utc)

Would you be up for maknig a PR to improve the set_meas_date Notes section to mention how to go from local to utc time?

heilerich commented 3 years ago

Well that removes the error and preservers the correct point in time, but it also removes the timezone information.

The utc_offset property will still be unset. If you care about the local time of recordings, which might be of interest in e.g. sleep studies, you have no way of recovering that information anymore. That is what the utc_offset property is for, right?

At this point we are right at the beginning: We need to set utc_offset manually (which is deprecated according to the printed warning).

I think the question that needs to be answered is: What is the 'right' way to set the utc_offset, via the method or setting it directly? Then either the warning needs to be removed or set_meas_date needs an update.

larsoner commented 3 years ago

What is the 'right' way to set the utc_offset, via the method or setting it directly? Then either the warning needs to be removed or set_meas_date needs an update.

We could probably make it so that set_meas_date accepts non-UTC (but would still need to be tz-aware) datetime objects, and if it's not UTC it would set the utc_offset parameter. I don't think anything would break because of this

heilerich commented 3 years ago

We could probably make it so that set_meas_date accepts non-UTC (but would still need to be tz-aware) datetime objects, and if it's not UTC it would set the utc_offset parameter. I don't think anything would break because of this

IMHO that is the most reasonable solution. If this is something that is likely to be accepted, I can send a PR.

On a side node: The current type of the offset seems a little awkward. It always requires string formatting / parsing before it can be used. Is there a good reason not to make this a python timezone instead? That way dealing with any datetime in a local format would be as easy as calling .astimezone(info['utc_offset'])

larsoner commented 3 years ago

Is there a good reason not to make this a python timezone instead? That way dealing with any datetime in a local format would be as easy as calling .astimezone(info['utc_offset'])

It's string because it's how it needs to be written to FIF as s00:00. This is something we can (and probably should) handle on read/write by converting to/from the str representation and the timezone one. I think we could consider this a bugfix.

@hoechenberger @agramfort @bloyl @sappelhoff you have worked on meas_date stuff with anonymization and BIDS I think, okay to:

  1. Allow set_meas_date to accept non-UTC times
  2. Convert any non-UTC input to UTC, and set utc_offset based on what the original timezone was
  3. Make info['utc_offset'] a timezone object (by converting to/from str, e.g. -07:00 for the US/Pacific timezone)

(FWIW I checked some FIF files we recently acquired at UW on TRIUX and it does look like the meas_date is in UTC and you need to add utc_offset to it to get to local time.)

sappelhoff commented 3 years ago

you have worked on meas_date stuff with anonymization and BIDS I think, okay to:

yes, I think this three step procedure makes sense 👍

in BIDS, timestamps may either have:

see: https://bids-specification.readthedocs.io/en/latest/02-common-principles.html#units

Date time information MUST be expressed in the following format YYYY-MM-DDThh:mm:ss[.000000][Z] (year, month, day, hour (24h), minute, second, optional fractional seconds, and optional UTC time indicator). This is almost equivalent to the RFC3339 "date-time" format, with the exception that UTC indicator Z is optional and non-zero UTC offsets are not indicated. If Z is not indicated, time zone is always assumed to be the local time of the dataset viewer. No specific precision is required for fractional seconds, but the precision SHOULD be consistent across the dataset. For example 2009-06-15T13:45:30

emphasis by me

--> so for MNE-BIDS, this would all be no problem, and I think it makes the situation clearer.

hoechenberger commented 3 years ago
  • no timezone indicator
  • a UTC timezone indicator (Z)

Best. Standard. Ever. 🙈🙈🙈

heilerich commented 3 years ago

UTC indicator Z is optional and non-zero UTC offsets are not indicated. If Z is not indicated, time zone is always assumed to be the local time of the dataset viewer.

Wouldn't that mean that the current MNE-BIDS implementation violates the standard? MNE currently mandates meas_date to be in UTC time. The current implementation disregards the information stored in utc_offset (if any) and just writes the UTC time to the dataset with the Z indicator.

https://github.com/mne-tools/mne-bids/blob/b24c28a8850fa97574124d36f27d11e209b7d068/mne_bids/write.py#L452

My interpretation of the quoted standard would be, that MNE-BIDS should look for utc_offset and convert meas_date into a local time before writing the BIDS files. If no offset is found the date can be written with the Z indicator.

Anyways, with the current implementation the BIDS dataset looses the information about the local time of the recording. I would say that is a problem, because that information could be of interest itself (e.g. sleep studies) and associated data (like protocols etc.) are most likely to be in local time in my experience.

sappelhoff commented 3 years ago

Wouldn't that mean that the current MNE-BIDS implementation violates the standard?

I don't see how. MNE-BIDS writes UTC time with Z indicator (= zero offset). Any information about offsets (i.e., local info on where the data was recorded) is thus removed.

My interpretation of the quoted standard would be, that MNE-BIDS should look for utc_offset and convert meas_date into a local time before writing the BIDS files. If no offset is found the date can be written with the Z indicator.

BIDS doesn't prescribe that datetimes must be written in local time

Anyways, with the current implementation the BIDS dataset looses the information about the local time of the recording.

indeed, and if I (and probably Richard) could re-write the standard, we'd do many many things differently. But as it is, this is what we have with BIDS, and there are ways with which you can encode the location where data was recorded. For example with an extra column in your scans.tsv file, which is then documented in your scans.json file.


Having said that, I still think the proposal for MNE-Python is good

agramfort commented 3 years ago

to be honest I had never paid attention to utc_offset ...

so for sure there is something to be fixed...

heilerich commented 3 years ago

@sappelhoff I guess you are right, it is not a violation of the standard. I would still argue, that at least giving users the option to write dates in local time (which the standard allows), but that discussion is probably better suited for the MNE-BIDS repository.

Regarding MNE itself, if everyone is in favor I will see if I can find some time to implement the changes proposed by @larsoner https://github.com/mne-tools/mne-python/issues/9953#issuecomment-961158494

szerfas1 commented 2 years ago

I think you just need to add a .astimezone(utc), like:

>>> date = datetime.now().replace(tzinfo=timezone(timedelta(hours=2), 'Europe/Berlin'))
>>> date
datetime.datetime(2021, 11, 4, 9, 47, 12, 306954, tzinfo=datetime.timezone(datetime.timedelta(seconds=7200), 'Europe/Berlin'))
>>> date.astimezone(timezone.utc)
datetime.datetime(2021, 11, 4, 7, 47, 12, 306954, tzinfo=datetime.timezone.utc)

Would you be up for maknig a PR to improve the set_meas_date Notes section to mention how to go from local to utc time?

Also heads up that the current system doesn't work with pytz. I got a confusing error message until I came here.

naive_start_time_datetime = datetime.fromtimestamp(start_time_epoch)
local = pytz.timezone("America/Los_Angeles")
local_dt = local.localize(naive_start_time_datetime)
utc_dt = local_dt.astimezone(pytz.utc)

Gave error:

Traceback (most recent call last):
  File "/Users/stephenzerfas/src/jhana_proj/process_node_data.py", line 194, in <module>
    main()
  File "/Users/stephenzerfas/src/jhana_proj/process_node_data.py", line 181, in main
    mne_raw = convert_to_mne_raw_array(data_dicts)
  File "/Users/stephenzerfas/src/jhana_proj/process_node_data.py", line 106, in convert_to_mne_raw_array
    raw.set_meas_date(utc_dt)
  File "/Users/stephenzerfas/src/jhana_proj/conda-env/lib/python3.9/site-packages/mne/channels/channels.py", line 654, in set_meas_date
    meas_date = _handle_meas_date(meas_date)
  File "/Users/stephenzerfas/src/jhana_proj/conda-env/lib/python3.9/site-packages/mne/annotations.py", line 690, in _handle_meas_date
    _check_dt(meas_date)  # run checks
  File "/Users/stephenzerfas/src/jhana_proj/conda-env/lib/python3.9/site-packages/mne/utils/numerics.py", line 1029, in _check_dt
    raise ValueError('Date must be datetime object in UTC: %r' % (dt,))
ValueError: Date must be datetime object in UTC: datetime.datetime(2022, 3, 21, 22, 35, 37, 39000, tzinfo=<UTC>)

But this works:

start_time_epoch = ensure_epoch_in_seconds(start_time_epoch)
naive_start_time_datetime = datetime.fromtimestamp(start_time_epoch)
local = pytz.timezone("America/Los_Angeles")
local_dt = local.localize(naive_start_time_datetime)
utc_dt = local_dt.astimezone(tz=timezone.utc)