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

Disable automatic coloring and sizing on Points #748

Merged
merged 5 commits into from
Mar 11, 2017
Merged

Conversation

philippjfr
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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

@jlstevens
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
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
Copy link
Member Author

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
Copy link
Member

jbednar commented Jul 1, 2016

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

@philippjfr
Copy link
Member Author

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

@redst4r
Copy link

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
Copy link
Member Author

@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
Copy link
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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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

@philippjfr
Copy link
Member Author

Going to move forward on this.

@philippjfr philippjfr force-pushed the change_defaults branch 2 times, most recently from 5e12524 to 8e0b179 Compare February 23, 2017 17:39
@jlstevens
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

jbednar commented Mar 10, 2017

Excellent, thanks!

@philippjfr
Copy link
Member Author

@jlstevens Finally ready to merge.

@jlstevens
Copy link
Contributor

Great! Glad this is finally done.

@jlstevens jlstevens merged commit 07a6752 into master Mar 11, 2017
@jbednar jbednar deleted the change_defaults branch April 15, 2017 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants