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

Disable automatic coloring and sizing on Points #748

Merged
merged 5 commits into from Mar 11, 2017

Conversation

Projects
None yet
4 participants
@philippjfr
Member

philippjfr commented Jul 1, 2016

This is a continuing source of confusion and I'm suggesting we disable the color_index and size_index on Points by default. Having it magically map your variables to colors or scaling the points is often weird and undesired until you have actually requested it. We should also define a default colormap for Points in both backends so that if you do set a color_index it is actually applied in both cases (matplotlib would just fall back to the default cmap before). I'd be open to enabling the color_index by default but sizing can have very unpredictable behavior and should not just happen automatically.

Discussion needed.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 1, 2016

This is a continuing source of confusion and I'm suggesting we disable the color_index and size_index on Points by default. Having it magically map your variables to colors or scaling the points is often weird and undesired until you have actually requested it.

I agree. My worry is backwards compatibility.

We should also define a default colormap for Points in both backends so that if you do set a color_index it is actually applied in both cases (matplotlib would just fall back to the default cmap before).

As far as I know, our only other default colormap is 'hot'. Anything that shows up well against 'hot' would be good.

I'd be open to enabling the color_index by default but sizing can have very unpredictable behavior and should not just happen automatically.

Hmm. Actually, I might prefer this. Sizing is definitely problematic but automatically using a colormap doesn't seem so bad. As a result, I might just consider disabling size_index and not color_index.

Unfortunately, size_index seems to have had higher priority than color_index. For this reason, disabling both might be less confusing from a backwards compatibility perspective.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 1, 2016

I agree. My worry is backwards compatibility.

We've only had sizing enabled by default since 1.5 so going back on that shouldn't be too bad.

As far as I know, our only other default colormap is 'hot'. Anything that shows up well against 'hot' would be good.

Yes, we can be consistent.

Hmm. Actually, I might prefer this. Sizing is definitely problematic but automatically using a colormap doesn't seem so bad. As a result, I might just consider disabling size_index and not color_index.

Unfortunately, size_index seems to have had higher priority than color_index. For this reason, disabling both might be less confusing from a backwards compatibility perspective.

Yes not sure what to do about that. We could leave color_index as it is for now, it will kick in less often anyway because Points do not always have 4 dimensions.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 1, 2016

Yes not sure what to do about that. We could leave color_index as it is for now, it will kick in less often anyway because Points do not always have 4 dimensions.

That would be odd though... x,y (skip a dimension) color?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 1, 2016

True, not sure what the best suggestion is. Perhaps setting both to None is most reasonable afterall.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 1, 2016

It depends on how often people would have relied on the default behaviour. That is hard to gauge though.

If I could ignore backwards compatibility, I would want color_index to be set on the first value dimension by default.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 1, 2016

If I could ignore backwards compatibility, I would want color_index to be set on the first value dimension by default.

Should we do that in 1.6? Not sure what to do now to make it consistent. Defining a cmap on Points in both backends will do that at least.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 1, 2016

Hmm, maybe set both to None for this release. Then some people might have to explicitly set the color_dim and then in 1.6 we can make the color_index behaviour default again?

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 1, 2016

Then some people might have to explicitly set the color_dim and then in 1.6 we can make this behaviour default again?

I'd prefer not changing the behavior twice.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 1, 2016

I'd prefer not changing the behavior twice.

Well, once it is set explicitly, that specification will be obeyed. I don't think re-enabling default behaviour after disabling it should cause much trouble...

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 1, 2016

Well, once it is set explicitly, that specification will be obeyed. I don't think re-enabling default behaviour after disabling it should cause much trouble...

Unless you don't want point coloring and in 1.6 it would then enable itself automatically.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 1, 2016

Unless you don't want point coloring and in 1.6 it would then enable itself automatically.

Hmm, if you have extra dimensions in your data, then yes.

I suppose you are right, we should decide right now. Maybe we can do this:

  • Make color_index act on the first value dimension.
  • In the release notes give the line modifying the parameter to restore the old behaviour for people who are relying on it.
@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 1, 2016

Make color_index act on the first value dimension.
In the release notes give the line modifying the parameter to restore the old behaviour for people who are relying on it.

Sounds reasonable, will have to comb through all the notebooks to make sure their output is unchanged.

@jbednar

This comment has been minimized.

Member

jbednar commented Jul 1, 2016

This is all very tricky. It was quite disorienting and confusing when 1.5 enabled colors and sizing, thereby changing existing behavior on most of my own notebooks.

On balance, I think I favor turning them both off by default, and making people explicitly map to color or size when they want. That will restore compatibility to pre-1.5 notebooks, leaving only ones written for 1.5 needing any change. Hopefully people who just made such notebooks still remember how to fix them, and even then the only ones that would need to change are those using colors and sizing implicitly.

I was about to say that any such change should be in a 1.6 release, but now that I think about it, it's arguably more appropriate for a 1.5.x release, because then it can be considered a bugfix to the 1.5 release, restoring pre 1.5 behavior.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 1, 2016

when 1.5 enabled colors and sizing, thereby changing existing behavior on most of my own notebooks.

That's not quite what happened, v1.5 enabled sizing by default because sizing changed and previous scaling_factor=1 disabled size scaling. Coloring was always on set to color_index=3 by default (except for bokeh), where no default colormap is defined and it aborts.

I was about to say that any such change should be in a 1.6 release, but now that I think about it, it's arguably more appropriate for a 1.5.x release, because then it can be considered a bugfix to the 1.5 release, restoring pre 1.5 behavior.

Disabling both wouldn't be a bug fix, but the previous behavior was weird, because sizing was ignored until you set the scaling_factor and the color_index was set to the second value dimension. I agree that disabling both by default is the most consistent behavior, just not sure whether to make that change now.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jul 1, 2016

I agree this is tricky which is why I wouldn't rush to merge this. I don't think this PR should be part of the dev2 release, but I do think we need to make a decision before the next official release.

@jbednar

This comment has been minimized.

Member

jbednar commented Jul 1, 2016

So the color behavior hasn't changed across the releases? I thought I remembered changes in both color and size, but if it was only size that changed, that's a good argument for making color continue to be enabled. We definitely do need to make bokeh consistent with matplotlib for coloring, though.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jul 1, 2016

So the color behavior hasn't changed across the releases? I thought I remembered changes in both color and size, but if it was only size that changed, that's a good argument for making color continue to be enabled. We definitely do need to make bokeh consistent with matplotlib for coloring, though.

I don't think it has, may be wrong though. The problem is it was set to color_index=3, which is weird if we're going to enable it by default, so setting it color_index=2 would be a significant change in behavior.

@jbednar

This comment has been minimized.

Member

jbednar commented Jul 1, 2016

I think disabling both sounds safest, then. Sounds like 1.6, though.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Aug 18, 2016

Are we still doing this (disabling both by default) and should it be in 1.6.2?

@redst4r

This comment has been minimized.

redst4r commented Sep 22, 2016

+1 for setting a default colormap in bokeh.
It was quite confusing for me that the following wont show colored circles in bokeh, while it worked fine in matplotlib:

%%opts Points [color_index=2 size_index=3 scaling_factor=50]
data = np.random.rand(100,4)
points = hv.Points(data, vdims=['z', 'alpha'])

works in bokeh when explicitly specifying a colormap. But this took me a while to figure out... (I thought the bokeh backend is somehow broken here...)

%%opts Points [color_index=2 size_index=3 scaling_factor=50] (cmap='jet')
@philippjfr

This comment has been minimized.

Member

philippjfr commented Sep 22, 2016

@redst4r Thanks for the input. This should have priority for 1.7.0, the behavior has been confusing for quite some time. Our conclusion was to disable color_index and size_index by default but the alternative would be to set color_index=2 and size_index=3 and add a default colormap for bokeh. What's your preference?

@philippjfr philippjfr added this to the v1.7.0 milestone Sep 22, 2016

@jbednar

This comment has been minimized.

Member

jbednar commented Sep 22, 2016

Whether disabling coloring and sizing by default or not, adding a default colormap for Bokeh seems like a good idea.

@redst4r

This comment has been minimized.

redst4r commented Sep 23, 2016

i'd be fine with disabling color_index and size_index.
I was just surprised that even when specifying color_index (but no colormap), bokeh didnt show me any colored points.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Oct 12, 2016

I now think color_index should always be None by default for all elements - otherwise it can be very mysterious why explicitly specified colors don't stick. Then if color_index is active, at least it has been explicitly requested by the user to override color.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Nov 8, 2016

I just made a tiny commit to trigger a fresh travis build. Hopefully this will allow us to update the test data.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 23, 2017

Going to move forward on this.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Feb 23, 2017

This has been a long thread - to summarize, we are setting these indices to None by default right?

I now think it is better to be explicit especially as having a default color index can mask explicit attempts to set colors.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Feb 23, 2017

This has been a long thread - to summarize, we are setting these indices to None by default right?

Correct, I just have to update a few tutorial examples to explicitly set these now and then update the test data for a few plots which didn't have a default colormap defined (and fell back to jet).

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 10, 2017

So I just went through all the tutorials and they were already being explicit about setting size_index and color_index. I'm rebuilding the test data and then let's finally get this merged.

@jbednar

This comment has been minimized.

Member

jbednar commented Mar 10, 2017

Excellent, thanks!

@philippjfr

This comment has been minimized.

Member

philippjfr commented Mar 11, 2017

@jlstevens Finally ready to merge.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Mar 11, 2017

Great! Glad this is finally done.

@jlstevens jlstevens merged commit 07a6752 into master Mar 11, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on change_defaults at 78.495%
Details
s3-reference-data-cache Test data is cached.
Details

@jbednar jbednar deleted the change_defaults branch Apr 15, 2017

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