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

Confusing filter specification in EEG, MEG, iEEG #339

Closed
sappelhoff opened this issue Sep 23, 2019 · 10 comments · Fixed by #348
Closed

Confusing filter specification in EEG, MEG, iEEG #339

sappelhoff opened this issue Sep 23, 2019 · 10 comments · Fixed by #348

Comments

@sappelhoff
Copy link
Member

sappelhoff commented Sep 23, 2019

Intro to the issue

We are currently having two temporal filter related fields in the electrophysiology specification:

  • SoftwareFilters
  • HardwareFilters

both fields have a description as follows (with slight adjustments), please focus on the specification of filters:

list of temporal software filters applied. Ideally key:value pairs of pre-applied software filters and their parameter values: e.g., {"Anti-aliasing filter": {"half-amplitude cutoff (Hz)": 500, "Roll-off": "6dB/Octave"}}. Write n/a if no software filters applied.

So we can always use a string "n/a".

Beyond that, the information is a bit confusing, as identified by @effigies in https://github.com/bids-standard/bids-specification/pull/322/files#r321275320.

We are asking for a list, which brings to mind an array of the following form:

{
    "myKey": ["val1", "val2", 45455.3, {"obj": 1}]
}

However, in the validator and our examples (e.g., starter kit), we are requiring an "object of objects" in the form:

{
    "HardwareFilters": {"HighpassFilter": {"CutoffFrequency": 0.1}}
}

questions

Should we:

  1. stay with our current way to specify filters and simply clarify the language in the specification to drop the word "list", and use "object of objects" or similar?
  2. change the way we specify filters to be a "list of objects" --> this would allow the benefit to intuitively specify the order of how filters were applied to the data
  3. allow both and change validator and spec text accordingly

I am not a signal processing expert ... but as long as the order in which filters are applied is irrelevant (e.g., because they are linear operations), I am in favor of option 1.

@effigies
Copy link
Collaborator

Related: #322

@effigies
Copy link
Collaborator

I think either 1 or 2 is fine. I think having too much flexibility is a validation nightmare so I would recommend against 3. You already have the ability to have a string or an object with one entry. The question is whether multiple entries involve an object with multiple entries or a list of objects with one entry each.

I don't really have a dog in this fight, as I don't ever try to use *EG data. My inclination is that ordering is good to specify even when it isn't critically important. (Also that linear filters are mathematically exchangeable on real numbers, but floating point numbers are not reals, and error accumulates.)

@CPernet
Copy link
Collaborator

CPernet commented Oct 11, 2019

given #322 is this one resolved? ie use of a list

@effigies
Copy link
Collaborator

I don't think so. #322 punted on the issue, improving the docs, but not actually addressing whether we use a list of objects or a multi-entry object.

@CPernet
Copy link
Collaborator

CPernet commented Oct 11, 2019

I vote list of objects @sappelhoff @jasmainak @robertoostenveld any pref?

@robertoostenveld
Copy link
Collaborator

I vote list of objects.

@jasmainak
Copy link
Collaborator

jasmainak commented Oct 12, 2019 via email

@sappelhoff
Copy link
Member Author

For being pragmatic, I vote "object of objects" ... if the order is important, it can be specified with a custom key value pair inside the object. This would mean only a specification clarification within the text.

going the "list of objects" way will require:

  • changing spec
  • adjusting validator
  • fixing existing bids-examples
  • fixing examples on bids-starter kit
  • fixing openneuro datasets
  • acknowledging that several user datasets will now suddenly be invalid although they were valid before

@jasmainak
Copy link
Collaborator

On second thoughts, I agree with @sappelhoff . Backwards compatibility is important.

@CPernet
Copy link
Collaborator

CPernet commented Oct 16, 2019 via email

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 a pull request may close this issue.

5 participants