Skip to content

refactor plot_epochs_image #6329

Description

@drammock

There was a discussion in #6288 about refactoring plot_epochs_image. Here is the proposed new API (copied with clarifications from #6288):

+--------------------+----------+-------------+----------------------------+
| picks              | group_by | combine     | result                     |
+====================+==========+=============+============================+
|                    |          | callable    |                            |
|                    |          +-------------+                            |
|                    |          | 'mean'      |                            |
|                    |          +-------------+                            |
|                    | None     | 'median'    | 1 fig per ch_type          |
|                    |          +-------------+                            |
|                    |          | 'std'       |                            |
|                    |          +-------------+                            |
| None,              |          | 'gfp', None |                            |
| ch_type,           +----------+-------------+----------------------------+
| list of ch_types   |          | callable    | 1 fig per dict key         |
|                    |          +-------------+                            |
|                    |          | 'mean'      | (dict values index into    |
|                    |          +-------------+  orig. sensor indices, not |
|                    | dict     | 'median'    |  whatever `picks` yields)  |
|                    |          +-------------+                            |
|                    |          | 'std'       | (if one dict value selects |
|                    |          +-------------+  multiple sensor types:    |
|                    |          | 'gfp', None |  raise ValueError)         |
+--------------------+----------+-------------+----------------------------+
|                    |          | callable    |                            |
|                    |          +-------------+                            |
|                    |          | 'mean'      | 1 fig                      |
|                    |          +-------------+                            |
|                    | None     | 'median'    | (stop allowing to combine  |
|                    |          +-------------+  across sensor types:      |
|                    |          | 'std'       |  Warning -> ValueError)    |
|                    |          +-------------+                            |
|                    |          | 'gfp'       |                            |
| int,               |          +-------------+----------------------------+
| list of int,       |          | None        | 1 fig per pick             |
| ch_name,           +----------+-------------+----------------------------+
| list of ch_names   |          | callable    | 1 fig per dict key         |
|                    |          +-------------+                            |
|                    |          | 'mean'      | (dict values index into    |
|                    |          +-------------+  orig. sensor indices, not |
|                    | dict     | 'median'    |  whatever `picks` yields)  |
|                    |          +-------------+                            |
|                    |          | 'std'       | (if one dict value selects |
|                    |          +-------------+  multiple sensor types:    |
|                    |          | 'gfp', None |  raise ValueError)         |
+--------------------+----------+-------------+----------------------------+

I think this table summarizes what the behavior should be. It's different from
current behavior only in two cases:

  1. when picks is a (list of) channel types, and group_by and combine are
    both None. Currently in that case you get 5 arbitrary channels; as
    proposed in table that returns 1 fig per ch_type (GFP).

  2. when picks is a (list of) ints or ch_names, group_by is None, and
    combine is not None, do not combine across sensor types anymore (current
    behavior is to emit a RuntimeWarning; change to ValueError). This is to
    be consistent with the other case (when picks=specific_indices_or_names
    and group_by=dict(...) selects heterogeneous sensor types, which already
    throws ValueError).

Given that spec, I think some changes that make sense are:

  • we don't need to accept group_by='type' anymore because it never gives a
    different result from group_by=None

  • Three related changes:

    1. Don't allow non-None for picks if group_by is a dict. Just infer
      picks from the dict values.
    2. when group_by dict values are integers, treat them as indexing into
      the full list of ch_names (necessary given previous point).
    3. allow group_by dict values to be channel names, not just integers
      (optional, but would be nice)
  • whenever group_by is None, construct a dict anyway depending on values of
    picks and combine. That should simplify the code path, because we'd
    always be plotting from a dict (one figure per dict key).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions