[MRG+1] Contour levels to colorbar.

[jaeilepp]

Closes #3795

The colorbars are so small by default, that I had to make the tick labels a bit smaller.

[jaeilepp]

[jona-sassenhagen]

Warning: it is possible, and often desirable, to set 0 contour levels. Wouldn't this then mean you have no units on the colorbar?

Otherwise, you're a beast!

[jaeilepp]

Then it only shows the zero, but maybe this should also show vmax and vmin.

[jaeilepp]

Ok. Tried it, and it looks a bit weird, since it's not evenly spaced.

[jaeilepp]

I'm not sure which is the best way to do this. Opinions?

[codecov-io]

Current coverage is 86.38% (diff: 92.30%)

Merging #3797 into master will increase coverage by <.01%

@@             master      #3797   diff @@
==========================================
  Files           343        343          
  Lines         61271      61292    +21   
  Methods           0          0          
  Messages          0          0          
  Branches       9381       9384     +3   
==========================================
+ Hits          52929      52948    +19   
- Misses         5625       5626     +1   
- Partials       2717       2718     +1   

Powered by Codecov. Last update 07102b9...6833b0c

[jona-sassenhagen]

Make it an alternative option IMO.

[jaeilepp]

This made me realize that the contour levels aren't actually same for all the images.

[mmagnuski]

I also think this shouldn't be the default. For multiple topomaps contour levels would have to be forced to be the same... BTW - how does it look like if you do it the other way: instead of forcing colorbar ticks to be where the contour lines are, force contour lines to be where colorbar ticks have been set?

[jaeilepp]

Yeah, that's what I'm looking at now. Trying to find out how mpl does it. I don't want the contours to set to something like 166.6666666.

[mmagnuski]

I don't remember ever seeing mpl setting ticks to such weird values. :) But you can access the tick locators in ticker module. I've never played with these classes though...

[jaeilepp]

Now it's using the ticker levels and seems to work quite nicely. Here's how it looks with 3 contours.

[jona-sassenhagen]

That looks quite good IMO!

A somewhat more far-fetched idea, could similar tech be used to threshold certain t score maps by significance levels eventually ..?

[jaeilepp]

Probably. I don't know the stats package that well.

This changes the contour levels a little bit as can be seen from the pictures, but I suppose it's okay since the threshold levels were not previously visible anyway.

[mmagnuski]

Looks good!

[agramfort]

LGTM

[jona-sassenhagen]

"equals contours + 1" or "is equal to contours +1"

[agramfort]

LGTM

can someone else have a look so @jaeilepp can have his +2?

[jona-sassenhagen]

I will once I'm within wireless range again.

[larsoner]

Sorry to come late, but I actually think that showing the max/min and all the contour levels is cleanest. Even if it's not evenly spaced it's most informative in terms of what everything means in the plot

[jaeilepp]

It also looks quite bad in most cases


I would omit vmin and vmax.

[larsoner]

Yeah that does look bad, and if it's fairly common that it happens then it's not worth it. I suspect it wouldn't be too difficult to come up with a heurestic for when to include hte vmin/vmax based on where the other contours are, but that can be another PR if someone gets bored.

LGTM, @jona-sassenhagen feel free to merge if you're happy

[jaeilepp]

Yeah, I would have thought that there would have been an option for this in mpl's locator, but I couldn't find one that works.

[jona-sassenhagen]

So we're still set on not having this as a parameter-accessible option? It's gonna become the new default?

[jaeilepp]

I'm voting for this as a default. The other options would be more or less arbitrary values.

[mmagnuski]

I think this should be accessible through a keyword arg... (contour_legend, contour_colorbar?)

[jona-sassenhagen]

It's not easily user-changeable, isn't it? Which would mean it should be settable with a kwarg.

But I see your point @jaeilepp that the current method is arbitrary and maybe it's not so good to set "arbitrary" as a param.

[jaeilepp]

Actually, plot_topomap now also accepts a list for contours. So maybe we can live without a new kwarg and just update the documentation.

[mmagnuski]

I like the fact that contours are accepted. I was just a little afraid that if colorbar tick positioning is contour-dependent by default, then colorbar will easily get messy with many contour levels. I mean - it can be the default behavior, but it would be nice if it was possible to turn it off.

[agramfort]

what do you suggest? is this good for you to merge?

[mmagnuski]

My suggestion (and Jona's) was to add a keyword argument that allows to turn off this behavior, but make it default. @jaeilepp - would that be ok?

[larsoner]

@mmagnuski can we add that once we hit a use case that needs it? Or do you already know there will be?

[mmagnuski]

@Eric89GXL sure, I've played with it a bit now and I no longer think that colorbar will get messy for a reasonable number of contour levels. :)

[agramfort]

so you give your +1?

[jona-sassenhagen]

Well it has my +1 :D

Tried it, looks good.

[mmagnuski]

Yes, 👍 from me too :)

[larsoner]

Thanks @jaeilepp

[jona-sassenhagen]

Thanks @jaeilepp !

[mmagnuski]

Thanks @jaeilepp !!