Skip to content

Conversation

kingjr
Copy link
Member

@kingjr kingjr commented Jul 25, 2022

I encountered a bug when reading a CTF dataset with an empty string in the res4['data_date'] and res4['data_time'] fields.

This sets the field to January 1st of the year 1001 (earlier make a date parsing bug)

@larsoner
Copy link
Member

I would rather set the date to 1) the epoch (0., 0.) in FIF form, or 2) None, but (2) probably makes working with things like annotations harder. (0., 0.) seems like a better choice than 1001. Or were you trying to avoid possible collisions with anonymized dates...? In that case I could be persuaded :)

@kingjr
Copy link
Member Author

kingjr commented Jul 26, 2022

new commit: I had to use a higher date, else there is an error at reading time after saving the ctf as a fif.

Thanks Eric. I did not understand option 1), but I agree that option 2) would be best: we don't have a date, so we shouldn't try to force it. However, it will likely require a lot of downstream corrections.

I'll be using this branch, as it unblocks my pipeline, but feel free to take over.

@larsoner
Copy link
Member

1970/01/01 okay for you @kingjr ?

@agramfort @hoechenberger @drammock feel free to merge if happy

@larsoner larsoner added this to the 1.1 milestone Jul 26, 2022
@kingjr
Copy link
Member Author

kingjr commented Jul 26, 2022

LGTM, thanks Eric!

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one tiny suggestion to make the logger.info message clearer.

Co-authored-by: Daniel McCloy <[email protected]>
@larsoner larsoner enabled auto-merge (squash) July 26, 2022 15:29
@larsoner larsoner merged commit c12a020 into mne-tools:main Jul 26, 2022
larsoner added a commit to alexrockhill/mne-python that referenced this pull request Jul 27, 2022
* upstream/main:
  FIX: ctf data can sometimes have no date time. (mne-tools#10957)
  replace check_version from _testing.py with the one in check.py (mne-tools#10958)
  ENH:  mne-tools#10542 Adds __eq__() to DigMontage class and updates tests. (mne-tools#10942)
  Use HiDPI icons with PyQt5/PySide2 (mne-tools#10956)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants