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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Colorscale drawing fix #1112

Merged
merged 3 commits into from Nov 10, 2016
Merged

Colorscale drawing fix #1112

merged 3 commits into from Nov 10, 2016

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Nov 4, 2016

fixes #1109

while 馃敧 a bunch of useless code 馃帀

- that made color array containing identical items
  not draw properly.
max = -Infinity;
colorArray.forEach(function(color) {
if(isNumeric(color)) {
if(min > color) min = +color;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block here used to override the Colorscale.calc computations in the case where marker.cauto (or marker.line.cauto) was true.

馃敧 馃敧 馃敧

Luckily, for most cases, this block here gave the same results as Colorscale.calc. One exception coming when the computated cmin and cmax where equal. In that case, Colorscale.calc spreads them apart by -0.5 and +0.5 respectively here whereas the removed block left them as is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds right, do you want to test it? Could just be adding a second scale to the mock you already edited...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call!

Copy link
Contributor Author

@etpinard etpinard Nov 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to test it?

Test what exactly? Just to be sure.

The equal cmin and cmax case is now tested via 860ccee

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test the colorscale we draw in this case, so it's clear what cmin and cmax end up being. That mock tests that the right markers show up (and I guess it's a bipolar colorscale so the center color is the same gray you would have gotten from a one-sided scale if this routine had chosen the starting color)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok. You mean show the colorbar. Good call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha yeah, colorscale, colorbar... you figured out what I meant :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done in 0539e28

@alexcjohnson
Copy link
Contributor

Nice. 馃拑

@etpinard etpinard merged commit 88dbc9d into master Nov 10, 2016
@etpinard etpinard deleted the colorscale-drawing-fix branch November 10, 2016 23:29
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.

scatter marker colorscale yield wrong color for all 0 color array
2 participants