mne-tools / fiff-constants

Bookkeeping and documentation of FIFF file format constants
4 stars 12 forks source link

BUG: Dates #24

Closed larsoner closed 4 years ago

larsoner commented 4 years ago

Dates are currently stored in meas_date in 32-bit (signed) integers. However, this is going to be problematic in 2038, since that is the latest date that can be stored using int32 seconds (see this issue). And in the linked issue, it seems like it could be problematic sooner/today, because of how BIDS standardization and anonymization works.

It would be good if we could store dates in 64-bit integers, which seems to be the C convention anyway. It looks like this is already a supported primitive.

@jnenonen what do you think about:

  1. Adding a meas_date_long tag
  2. In reading info structures, prefer to save and load the meas_date_long tag, but fall back to meas_date / int32 when the long one is not available?

I think this would be a few lines for us over in MNE, but wanted to see if this would be feasible for MEGIN, and if this seems like a reasonable approach to having larger dates.

cc @mshamalainen @agramfort in case you have thoughts/ideas as well

jnenonen commented 4 years ago

Hi Eric,

This is a known issue, but there may not be a quick workaround like adding new tag like meas_date_long. It would work in properly coded applications, but also break many other applications such as our own legacy software.

The best way would be to fix all 32-bit related problems in a major overhaul of the fiff-version. We have to do it sooner rather than later, but plan it very carefully because it will have major implications in all software handling FIFF-format files.

Unfortunately I cannot give any estimate when we could start such format overhauling, but let’s continue this discussion…

BR, Jukka

[NENONEN_Outlook Signature]

From: Eric Larson notifications@github.com Sent: 6. marraskuuta 2019 16:54 To: mne-tools/fiff-constants fiff-constants@noreply.github.com Cc: Jukka Nenonen jukka.nenonen@megin.fi; Mention mention@noreply.github.com Subject: [mne-tools/fiff-constants] BUG: Dates (#24)

Dates are currently stored in meas_date in 32-bit (signed) integers. However, this is going to be problematic in 2038, since that is the latest date that can be stored using int32 seconds (see this issuehttps://github.com/mne-tools/mne-python/pull/7016#issuecomment-550131763). And in the linked issue, it seems like it could be problematic sooner/today, because of how BIDS standardization and anonymization works.

It would be good if we could store dates in 64-bit integers, which seems to be the C convention anywayhttps://en.wikipedia.org/wiki/Integer_(computer_science)#Common_integral_data_types. It looks like this is already a supported primitivehttps://github.com/mne-tools/fiff-constants/blob/master/DictionaryTypes.txt#L118.

@jnenonenhttps://github.com/jnenonen what do you think about:

  1. Adding a meas_date_long tag
  2. In reading info structures, prefer to save and load the meas_date_long tag, but fall back to meas_date / int32 when the long one is not available?

I think this would be a few lines for us over in MNE, but wanted to see if this would be feasible for MEGIN, and if this seems like a reasonable approach to having larger dates.

cc @mshamalainenhttps://github.com/mshamalainen @agramforthttps://github.com/agramfort in case you have thoughts/ideas as well

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mne-tools/fiff-constants/issues/24?email_source=notifications&email_token=ADR3D6SJPMZ6RW7VKLBI52LQSLLBDA5CNFSM4JJWB3UKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HXIBJNA, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADR3D6V6JROGKTM3KOGPKXLQSLLBDANCNFSM4JJWB3UA.

larsoner commented 4 years ago

Okay I'll close this issue/idea then in favor of overhauling the 32-bit-ness more globally eventually.

mkajola commented 4 years ago

Hello,

The current dictionary (at least the copy I have here), has a bug in date types. It has probably been there from the start. They are marked as int32 while they are int32(2). I don’t think there should be tag meas_date_long, since the tags are specifying meanings, not encodings. If the long dates are needed, I would rather specify alternative encoding to the tag. Simplest would be int64(2). Unfortunately not all programs check what is the type of the data. This means that the change will break silently many things.

I would not change this at this point unless there is no real need for this. There is still some time to 2032. That C defines date as 64 bits is not a reason, since file encodings and software internal representations are different domains, and there is no fundamental reason why the encodings in both should be same. Assuming so is not good software design.

t. mjk

From: Jukka Nenonen notifications@github.com Sent: torstai 7. marraskuuta 2019 9.41 To: mne-tools/fiff-constants fiff-constants@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [mne-tools/fiff-constants] BUG: Dates (#24)

Hi Eric,

This is a known issue, but there may not be a quick workaround like adding new tag like meas_date_long. It would work in properly coded applications, but also break many other applications such as our own legacy software.

The best way would be to fix all 32-bit related problems in a major overhaul of the fiff-version. We have to do it sooner rather than later, but plan it very carefully because it will have major implications in all software handling FIFF-format files.

Unfortunately I cannot give any estimate when we could start such format overhauling, but let’s continue this discussion…

BR, Jukka

[NENONEN_Outlook Signature]

From: Eric Larson notifications@github.com<mailto:notifications@github.com> Sent: 6. marraskuuta 2019 16:54 To: mne-tools/fiff-constants fiff-constants@noreply.github.com<mailto:fiff-constants@noreply.github.com> Cc: Jukka Nenonen jukka.nenonen@megin.fi<mailto:jukka.nenonen@megin.fi>; Mention mention@noreply.github.com<mailto:mention@noreply.github.com> Subject: [mne-tools/fiff-constants] BUG: Dates (#24)

Dates are currently stored in meas_date in 32-bit (signed) integers. However, this is going to be problematic in 2038, since that is the latest date that can be stored using int32 seconds (see this issuehttps://github.com/mne-tools/mne-python/pull/7016#issuecomment-550131763). And in the linked issue, it seems like it could be problematic sooner/today, because of how BIDS standardization and anonymization works.

It would be good if we could store dates in 64-bit integers, which seems to be the C convention anywayhttps://en.wikipedia.org/wiki/Integer_(computer_science)#Common_integral_data_types. It looks like this is already a supported primitivehttps://github.com/mne-tools/fiff-constants/blob/master/DictionaryTypes.txt#L118.

@jnenonenhttps://github.com/jnenonen what do you think about:

  1. Adding a meas_date_long tag
  2. In reading info structures, prefer to save and load the meas_date_long tag, but fall back to meas_date / int32 when the long one is not available?

I think this would be a few lines for us over in MNE, but wanted to see if this would be feasible for MEGIN, and if this seems like a reasonable approach to having larger dates.

cc @mshamalainenhttps://github.com/mshamalainen @agramforthttps://github.com/agramfort in case you have thoughts/ideas as well

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mne-tools/fiff-constants/issues/24?email_source=notifications&email_token=ADR3D6SJPMZ6RW7VKLBI52LQSLLBDA5CNFSM4JJWB3UKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HXIBJNA, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADR3D6V6JROGKTM3KOGPKXLQSLLBDANCNFSM4JJWB3UA.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/mne-tools/fiff-constants/issues/24?email_source=notifications&email_token=AKJMTHCNFBH3G6IZP5JI7ELQSPBCLA5CNFSM4JJWB3UKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDLQG4A#issuecomment-550962032, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AKJMTHBEHQUXIJCAMGHVCUDQSPBCLANCNFSM4JJWB3UA.