Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove daysback option #160

Closed
wants to merge 2 commits into from

Conversation

hoechenberger
Copy link
Member

Anonymization should happen on the BIDS level.

Closes GH-159.

@hoechenberger
Copy link
Member Author

CI fails:

RuntimeError: info[meas_id][secs] must be between "-2147483648" and "2147483647", got "-5364627420"

I remember, there was this issue with the previous anonymization approach in old datasets…

@hoechenberger
Copy link
Member Author

I first thought this was related to mne-tools/mne-python#7711 but it's probably sth else

@hoechenberger
Copy link
Member Author

hoechenberger commented Jul 16, 2020

I guess we'll just have to re-anonymize the dataset and push it to OpenNeuro? However I don't have write access to the ds000246 repo

It says "uploaded by Julia Guiomar Niso Galán" -- anyone know how I could get in touch to ask for creation of a new revision, or for access rights to publish it myself?

@agramfort
Copy link
Member

yes data needs to be fixed on openneuro

@jasmainak
Copy link
Member

ping @guiomar

@hoechenberger can you articulate what exactly is the problem with the dataset for the benefit of @guiomar ?

@hoechenberger
Copy link
Member Author

@jasmainak I just dropped her an email 😃 So my understanding of the issue is that, in essence, the dataset was anonymized with an older version of MNE, and newer versions have difficulties dealing with that. We need to re-anonymize the dataset with MNE 0.20 to fix the problem.

@jasmainak
Copy link
Member

I doubt it was anonymized with MNE. Maybe with brainstorm ...

@agramfort
Copy link
Member

agramfort commented Jul 16, 2020 via email

@guiomar
Copy link
Contributor

guiomar commented Jul 16, 2020

Hi! If I correctly remember we did a face mask on T1w MRI (using open tool Face Masking, http://nrg.wustl.edu/software/face-masking (Milchenko and Marcus 2013)) and anonymize the acquisition date by adding a random number of hours (different for each subject but the same for each scan within a subject) as suggested by the BIDS spec at that time (1.0.2).

Happy to upload the new version.
(I also ping @ftadel just in case)

@agramfort
Copy link
Member

agramfort commented Jul 17, 2020 via email

@guiomar
Copy link
Contributor

guiomar commented Jul 17, 2020

Ok, I see! Thank you Alex

@guiomar
Copy link
Contributor

guiomar commented Jul 17, 2020

By the way, do you know if this has been updated to the BIDS spec?
This is what is now on the scans.tsv:
https://bids-specification.readthedocs.io/en/stable/03-modality-agnostic-files.html#scans-file
I guess is this same parameter value (acq_time) which is inside the fif and ds files, right?

@agramfort
Copy link
Member

@sappelhoff @jasmainak can you help me point to the right section in the bids standard?
yes to me it's the acq_time parameter

@sappelhoff
Copy link
Member

sappelhoff commented Jul 17, 2020

The "anonymization" rule details in BIDS were changed in November 2019, see: bids-standard/bids-specification#363 ... based on the discussion in this issue: bids-standard/bids-specification#360

The only change was:

To distinguish real dates from shifted dates always use year
-1900
+1925
or earlier when including shifted years.

So I guess @agramfort is right with what he said in #160 (comment) ... the data was probably anonymized to some date prior to 1900 ... and this now trips up the FIF file format, which can only store dates that are a bit later (not prior 1900)

edit: it's important to say though, that anonymizing BEFORE 1900 is not wrong per se (see wording in the spec: "1925 or earlier") ... it's basically a FIF problem.

@jasmainak
Copy link
Member

what I don't understand is how brainstorm deals with dates before 1900. @guiomar must have checked that the files can be read after anonymizing.

@jasmainak
Copy link
Member

just to be sure @hoechenberger are you dealing with fif files or CTF files?

@sappelhoff
Copy link
Member

they are .ds files as it seems 🤷

@guiomar
Copy link
Contributor

guiomar commented Jul 17, 2020

Thanks a lot @sappelhoff!!
I can't recall if we did that anonymizaton with Bst or with CTF software. Anyways, I guess that maybe for .ds files is not as problematic as for fif. That may be why we could work with them without problems.

@jasmainak
Copy link
Member

so CTF files are not subject to the same int32 issues as fif, it might be an MNE issue ...

@agramfort
Copy link
Member

agramfort commented Jul 18, 2020 via email

@jasmainak
Copy link
Member

oh boy ...

@guiomar
Copy link
Contributor

guiomar commented Jul 20, 2020

Thanks! Then if this is the case, BIDS should enforce to to use a year 1925, and remove the "or earlier" from the specs.
As it doesn't make sense to allow something that won't be compatible with all the file formats. Or at least make a note, so people can decide knowing it, specially in the MEG field. I think it could be useful.

@jasmainak
Copy link
Member

Then if this is the case, BIDS should enforce to to use a year 1925, and remove the "or earlier" from the specs.

The problem is that in longitudinal studies, people want to keep the relative time. So if you had recordings over 5 years, they want to keep this information.

but I agree that the 1925 year limit is too stringent. It doesn't make pragmatic sense and will hinder adoption of BIDS in MEG community

@jasmainak
Copy link
Member

According to HHS regulations, you only need to anonymize up to a year:

(C) All elements of dates (except year) for dates that are directly related to an individual, including birth date, admission date, discharge date, death date, and all ages over 89 and all elements of dates (including year) indicative of such age, except that such ages and elements may be aggregated into a single category of age 90 or older

@sappelhoff
Copy link
Member

sappelhoff commented Jul 20, 2020

You can always do a PR to bids-standard/bids-specification to insert a note that alerts people to the fact that "choosing a very early date (e.g., 1880) will negatively affect interoperability. --> because FIF does not support it."

AFAIK this is not a problem for any other data format.

@guiomar
Copy link
Contributor

guiomar commented Jul 20, 2020

Thanks! I think I'll do :)
Just out of curiosity, what's the limit to crash the fif?

@sappelhoff
Copy link
Member

Just out of curiosity, what's the limit to crash the fif?

I think @alexrockhill looked into this deeply at some point.

@jasmainak
Copy link
Member

some date in 1901 :)

it's basically 1st January 1970 - number of seconds that can be represented by int32 (=2,147,483,647)

@alexrockhill
Copy link

alexrockhill commented Jul 20, 2020

The datetime is stored in fif as a 32-bit integer which when converted into seconds before/after Jan 1, 1970 (time = 0) ranges from 1901 to 2040 or so (the numbers aren't exact) that's why we had to ask BIDS to move the anonymization date required up from before 1900 to before 1925.

@guiomar
Copy link
Contributor

guiomar commented Jul 20, 2020

Thanks a lot!!

Anonymization should happen on the BIDS level.

Closes mne-toolsGH-159.
hoechenberger added a commit to hoechenberger/mne-bids-pipeline that referenced this pull request Feb 17, 2021
hoechenberger added a commit that referenced this pull request Feb 17, 2021
* Remove anonymization option

Closes #159, #160

* logger.warn -> logger.warning
@hoechenberger
Copy link
Member Author

Closed via #257

@hoechenberger hoechenberger deleted the dont-anonymize branch February 17, 2021 11:52
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.

Remove anonymization
6 participants