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

[FIX] Add SoftwareFilters to EEG sidecar example #322

Merged
merged 1 commit into from
Oct 11, 2019

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Sep 5, 2019

Related to issue #321. Closes #321

Just a small addition to make eeg json side car example a valid one according to the specs.

sappelhoff
sappelhoff previously approved these changes Sep 5, 2019
@@ -163,6 +163,12 @@ Example:
"EEGPlacementScheme":"10 percent system",
"EEGReference":"single electrode placed on FCz",
"EEGGround":"placed on AFz",
"SoftwareFilters":{
Copy link
Collaborator

Choose a reason for hiding this comment

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

The spec says "List of temporal software filters applied.", and this is an object that does not have an inherent ordering. If ordering is important, than this should probably be lists of single-element objects. If the ordering is not important, then it can be kept as is, but I would suggest updating the language of the spec to change from "list", which has a meaning in JSON.

As an example of a list of single-element objects:

  "SoftwareFilters": [
    {
      "Anti-aliasing filter": {
        "half-amplitude cutoff (Hz)": 500,
        "Roll-off": "6dB/Octave"
      }
    },
    {
      "Other filter": {
        "param1": "val1",
        "param2": "val2"
      }
    }
  ],

Copy link
Member

Choose a reason for hiding this comment

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

So far, we have always dealt with objects, see also the schema implemented in the validator

and the examples in the starter-kit

A change of language in the spec is thus warranted IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And order does not matter? Because software that parses JSON is not obligated to provide key-value pairs in any particular order, or preserve it when copying metadata forward to derivatives.

Copy link
Member

@sappelhoff sappelhoff Sep 6, 2019

Choose a reason for hiding this comment

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

This was never discussed in detail. I think in most use cases the order of filters (order in which they are applied after one another) is irrelevant - but in cases where it IS relevant, perhaps a keyword "order": "first" ... could be used? --> This would however be more easily solved with a list that has a naturally defined order, as you mentioned above Chris.

cc @robertoostenveld for some third opinion

Copy link
Member

Choose a reason for hiding this comment

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

tagging more EEG folks: Please read this thread and leave your opinion on how we want to format filters in BIDS. Estimated time: <5minutes @bids-standard/raw-electrophys

Copy link
Member

@sappelhoff sappelhoff Sep 11, 2019

Choose a reason for hiding this comment

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

  1. As far as I understand it, JavaScript Arrays are a bit like Python lists.
  2. yes, the idea is that:
    1. either we allow an object ("SoftwareFilters") which contains one object per filter used .... and these filter objects then have key value pairs for their parameters
    2. OR we allow a list ("SoftwareFilters"), which is empty if None were applied ... or it contains objects, each of which is corresponding to a filter. Then the order of objects in the list corresponds to the order in which the respective filters were applied to the data

The second of the ideas above allows for an easy way to specify the order in which filters were used. ... the first of the ideas is how it's currently implemented in the validator

I think a practical solution would be to allow both variants. and IF the order of filters is relevant, then we RECOMMEND that users take the list of objects (solution 2) rather than the object of objects (solution 1) way.

Opinions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jasmainak

  1. https://json.org/

JSON is built on two structures:

  • A collection of name/value pairs. In various languages, this is realized as an object, record, struct, dictionary, hash table, keyed list, or associative array.
  • An ordered list of values. In most languages, this is realized as an array, vector, list, or sequence.

Their grammar does refer to the structure as an array, so I suppose there's some ambiguity as to whether "list" is a reserved word in that sense. But it does have a common computer-science meaning, which is referenced above.

  1. Sure.

To the point, though: does ordering ever matter? If it does, then I would suggest that an actual list (or array) be used. If it does not, then we can refer to it as an object.

Copy link
Member

Choose a reason for hiding this comment

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

To the point, though: does ordering ever matter? If it does, then I would suggest that an actual list (or array) be used. If it does not, then we can refer to it as an object.

this is a signal processing question, as long as the filters are linear, then the order of application of the filters should not matter.

I am not sure about non-linear filters though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The example of an anti-aliasing filter without subsequent resampling does not make much sense for most acquisition systems. An anti aliasing filter would typically be implemented in hardware. I would prefer a better (i.e. a more common and more easily recognized) example.

Copy link
Member

@sappelhoff sappelhoff Sep 23, 2019

Choose a reason for hiding this comment

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

Example:

The Brain Products BrainAmp always samples the data at 5kHz, applying a 1kHz anti-aliasing filter. However, in the recording software settings, you can also select lower sampling rates (e.g., 1kHz).

--> If that is done, the software applies a "software filter" and downsamples the data (online) ... before saving it to disk.

This is reflected in the present example, which I find good :-)

If you prefer a better example, could you perhaps suggest one?

@jasmainak
Copy link
Collaborator

The link for circle CI artifacts is missing. Did you branch from latest master @Remi-Gau ?

@Remi-Gau
Copy link
Collaborator Author

This was branched from the latest at the time so 4c39c10fb a week ago or so.
Want me to rebase from the latest things?

@sappelhoff
Copy link
Member

@Remi-Gau this is the PR that Mainak is referring to #315 ... everything after this PR will have an additional "check" which is a bogus check that acts as a convenient link directly from GitHub to the built html pages on CircleCi.

So yes, a rebase would be nice!

@Remi-Gau
Copy link
Collaborator Author

hum... If there is a way to do a rebase and not force-push to the remote branch, I did not find it.

@effigies
Copy link
Collaborator

If there is a way to do a rebase and not force-push to the remote branch, I did not find it.

There is not. Anything that's not a fast-forward merge needs to be force-pushed.

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

@effigies I suggest to merge this PR, because it is improving the status quo.

We can open a separate issue where we can discuss the wording of the SoftwareFilters field, and whether it should be an array of objects, or an object of objects, or both permissible. See #339

@robertoostenveld
Copy link
Collaborator

If it were up to me, BIDS for raw electrophysiology should not have software filters at all, since the data would be stored/archived/shared in the original form. But in practice, shared data might be minimally processed, so I see the justification for this. I don't care too much about the format, but supporting both a single string and a list sound nice to the researchers. Both soft and hardware filters should be kept consistent between the different electrophys modalities.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Sure, we can merge this as an improvement.

@effigies effigies changed the title [FIX] add SoftwareFilters to EEG json example [FIX] Add SoftwareFilters to EEG sidecar example Sep 26, 2019
@effigies effigies merged commit 8a3bf89 into bids-standard:master Oct 11, 2019
@Remi-Gau Remi-Gau deleted the remi-fix_eeg_json_example branch April 14, 2020 12:42
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.

EEG json example is missing required software filter field
5 participants