mne-tools / mne-python

MNE: Magnetoencephalography (MEG) and Electroencephalography (EEG) in Python
https://mne.tools
BSD 3-Clause "New" or "Revised" License
2.76k stars 1.33k forks source link

What does plot_evoked_topomap do if layout is missing? #994

Closed TalLinzen closed 10 years ago

TalLinzen commented 10 years ago

From the docstring for Evoked.plot_topomap / plot_evoked_topomap:

        layout : None | str | Layout
            Layout name or instance specifying sensor positions (does not need
            to be specified for Neuromag data).

This implies that when the layout isn't specified (i.e. is None) the default is to assume that the machine is a Neuromag, is that correct? Or is the idea that when you have a Neuromag machine MNE can automatically figure out which specific model you have? At any rate this seems like a potentially dangerous default, perhaps it would be better to raise an exception if layout=None when the data isn't from a Neuromag machine?

Also, what's the best way to make a layout for my non-Neuromag machine?

dengemann commented 10 years ago

Tal, take a look at mne.find_layout which is internally used inside the viz functions

TalLinzen commented 10 years ago

I did, and I'm not sure what happens there. I guess there are two issues here -- first I want to make sure I understand what happens, and then I want to update the docstring so that it's clearer about what happens...

TalLinzen commented 10 years ago

As far as I can tell there is no .lout file for the machine I'm using in the MNE Python directory structure, so I guess I'm a little baffled that mne.find_layout manages to find a layout.

agramfort commented 10 years ago

can we reproduce the problem with a file from: https://github.com/mne-tools/mne-python/tree/master/mne/fiff/kit/tests/data?

TalLinzen commented 10 years ago

Wait, I'm not sure there's a problem, I'm just trying to understand what's happening. Is MNE generating the sensor layout on the fly somehow?

larsoner commented 10 years ago

@TalLinzen I did not implement these functions, but measurement files typically contain sensor positions, which could be used to generate a channel layout on the fly. I suspect that's what we do...

agramfort commented 10 years ago

for an MEG helmet it should not happen. It should try to find a default layout. But maybe christian did some black magic :)

teonbrooks commented 10 years ago

I know that when I tried plotting the projections with mne.viz.plot_projs_topomap with the same system, it yells at me about there not being a proper layout. This seems to be related to the plotting issue as well.

agramfort commented 10 years ago

I guess you just need to fix the find_layout function and you should be fine

TalLinzen commented 10 years ago

was that directed to @t3on or to me?

agramfort commented 10 years ago

the first who gets bored with "Science" :)

TalLinzen commented 10 years ago

I'm still not in a position to fix anything -- I'm not sure what even happens and what (if anything) needs to be fixed... The git blame for most of lines in find_layout point to @dengemann and @agramfort ;)

haribharadwaj commented 10 years ago

For Biosemi EEG files, when I specify a .hpts coordinates file during initial import, the layout seems to be generated on the fly from the [chs] key when plotting topomap.

dengemann commented 10 years ago

Hi all, sorry for coming late to the party. I'm sure I can help clarifying things. How this is supposed to work (and works as documented by our tests for BTI, VV, CTF systems and certain EEG data) is that a layout object is created on the fly based on the channel information that can be mapped to certain vendors. Currently, and AFAIK, KIT systems are not supported. I was thinking a few times that we should let this function fail gracefully instead of returning None, if auto detection does not yield any result.

TalLinzen commented 10 years ago

@dengemann, by this function you mean find_layout? and by fail gracefully you mean raise an exception? ;)

TalLinzen commented 10 years ago

to make this more concrete: what should be the expected behavior when the user calls plot_evoked_topomap on a KIT dataset? At the moment some layout is magically chosen (which actually seems reasonable). Do you think an exception should be raised instead?

dengemann commented 10 years ago

by this function you mean find_layout? and by fail gracefully you mean raise an exception? ;)

yes and yes

dengemann commented 10 years ago

to make this more concrete: what should be the expected behavior when the user calls plot_evoked_topomap on a KIT dataset? At the moment some layout is magically chosen (which actually seems reasonable). Do you think an exception should be raised instead?

yes, in the first place. If you like the automagically generated layout, we can add it for now as the default behavior for KIT. But find_layout should fail if it does not find anything.

TalLinzen commented 10 years ago

I agree. That would be much less mysterious than the current behavior. I can try to do this and submit a PR (or maybe you want to do this because you're familiar with the function)

The only missing piece of the puzzle is where the automatically generated layout for KIT comes from.

agramfort commented 10 years ago

if you send me a test fif file + a demo script and a .lay or .lout file I can do it quickly.

TalLinzen commented 10 years ago

what's the best way to send you the file? it's just 1MB

agramfort commented 10 years ago

Email

TalLinzen commented 10 years ago

ok, sent

TalLinzen commented 10 years ago

OK, to summarize the conclusions: when plot_evoked_topomap can't find a layout, it generates one automatically using viz._find_topomap_coords. I've changed the docstring for plot_evoked_topomap to reflect that. I'm still not 100% sure this is great behavior to just silently fall back on the automatic layout generation method. Maybe issue a warning if viz._find_topomap_coords is being called so the user knows to take the layout with a grain a salt?

TalLinzen commented 10 years ago

(thanks @agramfort for investigating this)

dengemann commented 10 years ago

As I said, a RuntimeError on not finding the layout would be cleaner to me.

TalLinzen commented 10 years ago

ok, let me open a PR so we can discuss this more concretely.

christianbrodbeck commented 10 years ago

Sorry for being late to this discussion. When adding topomap plotting I added a function that can create a more or less evenly spaced 2d sensor map based on the 3d coordinates in case no pre-specified layout is available. This map is only for topomap plotting and is not a layout with rectangular spaces that could be used for plotting channel graphs for each sensor.

agramfort commented 10 years ago

the idea is that if we can guess box sizes then we can move all the logic in find_layout and simplify a lot plotting functions

wmvanvliet commented 10 years ago

Here is my shot at such a function:

def _box_size(points):
    """ Given a series of points, calculate an appropriate box size.

    Parameters
    ----------
    points : array, shape = (n_points, [x-coordinate, y-coordinate])

    Returns
    -------
    points : array, shape = (n_points, [x-coordinate, y-coordinate])
        Rescaled points to fit in the [0, 1] range, centered on 0.5
    width : float
        Width of the box
    height : float
        Height of the box
    """
    # Scale points so they are centered at (0, 0) and extend [-0,5, 0.5]
    points = np.asarray(points)
    x_min, x_max = np.min(points[:,0]), np.max(points[:,0])
    y_min, y_max = np.min(points[:,1]), np.max(points[:,1])
    x_range = x_max - x_min
    y_range = y_max - y_min
    points[:,0] = (points[:,0] - (x_min + x_max)/2.) * 1./x_range 
    points[:,1] = (points[:,1] - (y_min + y_max)/2.) * 1./y_range

    xdiff = lambda a,b: np.abs(a[0] - b[0])
    ydiff = lambda a,b: np.abs(a[1] - b[1])
    dist = lambda a,b: np.sqrt(xdiff(a,b)**2 + ydiff(a,b)**2)

    # Find the closest two points A and B
    all_combinations = list(combinations(points, 2))
    closest_points_idx = np.argmin([dist(a, b) for a, b in all_combinations])
    a, b = all_combinations[closest_points_idx]

    # The closest points define either the max width or max height
    if xdiff(a, b) > ydiff(a, b):
        width = xdiff(a, b)

        # Find all subplots that could potentially overlap horizontally
        candidates = [c for c in combinations(points, 2) if xdiff(*c) < width]

        # Find an appropriate height so all none of the found subplots will overlap
        height = ydiff( *candidates[np.argmin([ydiff(*c) for c in candidates])] )
    else:
        height = ydiff(a, b)

        # Find all subplots that could potentially overlap vertically
        candidates = [c for c in combinations(points, 2) if ydiff(*c) < height]

        # Find an appropriate width so all none of the found subplots will overlap
        width = xdiff( *candidates[np.argmin([xdiff(*c) for c in candidates])] )

    # Some subplot centers will be at the figure edge. Shrink everything so it fits
    # in the figure.
    scaling = 1 / (1. + width)
    points *= scaling
    width *= scaling
    height *= scaling
    points += 0.5

    return points, width, height
agramfort commented 10 years ago

@wmvanvliet thanks for sharing. Can you send a PR if it solves your problem?

@christianbrodbeck looks good for you?

christianbrodbeck commented 10 years ago

I did not work on topographically ordered boxes, only on the topomaps...

agramfort commented 10 years ago

@wmvanvliet what's the way forward?

wmvanvliet commented 10 years ago

Currently, the functions that generate a layout on the fly (like make_eeg_layout) cannot estimate box_sizes, it's a parameter that must be supplied with much trial and error. I will create a PR that uses the function above to address this. I suggest we move any discussion then to the PR thread, as this is becoming off-topic.

agramfort commented 10 years ago

sounds great. Thats for taking a stab at this.

wmvanvliet commented 10 years ago

find it at #1545

agramfort commented 10 years ago

can we close this?