Skip to content

Don't mutate colorscale, cmin/cmax and zmin/zmax into user traces #3341

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

Merged
merged 12 commits into from
Dec 17, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
flip scale when reversescale:true just before making scaleFn
- that we don't mutate fullData[i].colorscale in defaults
  and or calc.
  • Loading branch information
etpinard committed Dec 17, 2018
commit 0a686dd56a5dfc302ce633883c75f3b86ca84f08
8 changes: 6 additions & 2 deletions src/components/colorbar/connect.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@
* LICENSE file in the root directory of this source tree.
*/


'use strict';

var drawColorbar = require('./draw');
var flipScale = require('../colorscale/helpers').flipScale;

/**
* connectColorbar: create a colorbar from a trace, using its module to
Expand Down Expand Up @@ -49,7 +49,11 @@ module.exports = function connectColorbar(gd, cd, moduleOpts) {

var cb = cd[0].t.cb = drawColorbar(gd, cbId);

cb.fillgradient(container.colorscale)
var scl = container.reversescale ?
flipScale(container.colorscale) :
container.colorscale;

cb.fillgradient(scl)
.zrange([container[moduleOpts.min], container[moduleOpts.max]])
.options(container.colorbar)();
};
2 changes: 1 addition & 1 deletion src/components/colorscale/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ module.exports = function colorScaleAttrs(context, opts) {
valType: 'boolean',
role: 'style',
dflt: false,
editType: 'calc',
editType: 'plot',
description: [
'Reverses the color mapping if true.',
effectDesc,
Expand Down
3 changes: 1 addition & 2 deletions src/components/colorscale/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
'use strict';

var Lib = require('../../lib');
var flipScale = require('./helpers').flipScale;

module.exports = function calc(gd, trace, opts) {
var fullLayout = gd._fullLayout;
Expand Down Expand Up @@ -89,7 +88,7 @@ module.exports = function calc(gd, trace, opts) {
else scl = gd._fullLayout.colorscale.sequentialminus;

// reversescale is handled at the containerOut level
doUpdate('colorscale', scl, container.reversescale ? flipScale(scl) : scl);
doUpdate('colorscale', scl);

// We pushed a colorscale back to input, which will change the default autocolorscale next time
// to avoid spurious redraws from Plotly.react, update resulting autocolorscale now
Expand Down
6 changes: 2 additions & 4 deletions src/components/colorscale/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,9 @@ module.exports = function colorScaleDefaults(traceIn, traceOut, layout, coerce,
var autoColorscaleDflt;
if(sclIn !== undefined) autoColorscaleDflt = !isValidScale(sclIn);
coerce(prefix + 'autocolorscale', autoColorscaleDflt);
var sclOut = coerce(prefix + 'colorscale');

// reversescale is handled at the containerOut level
var reverseScale = coerce(prefix + 'reversescale');
if(reverseScale) containerOut.colorscale = helpers.flipScale(sclOut);
coerce(prefix + 'colorscale');
coerce(prefix + 'reversescale');

if(!opts.noScale && prefix !== 'marker.line.') {
// handles both the trace case where the dflt is listed in attributes and
Expand Down
32 changes: 24 additions & 8 deletions src/components/colorscale/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,36 @@ function hasColorscale(trace, containerStr) {
/**
* Extract colorscale into numeric domain and color range.
*
* @param {array} scl colorscale array of arrays
* @param {number} cmin minimum color value (used to clamp scale)
* @param {number} cmax maximum color value (used to clamp scale)
* @param {object} cont colorscale container (e.g. trace, marker)
* - colorscale {array of arrays}
* - cmin/zmin {number}
* - cmax/zmax {number}
* - reversescale {boolean}
* @param {object} opts
* - cLetter {string} 'c' (for cmin/cmax) or 'z' (for zmin/zmax)
*
* @return {object}
* - domain {array}
* - range {array}
*/
function extractScale(scl, cmin, cmax) {
function extractScale(cont, opts) {
var cLetter = opts.cLetter;

var scl = cont.reversescale ?
flipScale(cont.colorscale) :
cont.colorscale;

// minimum color value (used to clamp scale)
var cmin = cont[cLetter + 'min'];
// maximum color value (used to clamp scale)
var cmax = cont[cLetter + 'max'];

var N = scl.length;
var domain = new Array(N);
var range = new Array(N);

for(var i = 0; i < N; i++) {
var si = scl[i];

domain[i] = cmin + si[0] * (cmax - cmin);
range[i] = si[1];
}
Expand All @@ -72,13 +90,11 @@ function extractScale(scl, cmin, cmax) {
function flipScale(scl) {
var N = scl.length;
var sclNew = new Array(N);
var si;

for(var i = N - 1, j = 0; i >= 0; i--, j++) {
si = scl[i];
var si = scl[i];
sclNew[j] = [1 - si[0], si[1]];
}

return sclNew;
}

Expand Down
2 changes: 1 addition & 1 deletion src/components/drawing/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ drawing.tryColorscale = function(marker, prefix) {

if(scl && Lib.isArrayOrTypedArray(colorArray)) {
return Colorscale.makeColorScaleFunc(
Colorscale.extractScale(scl, cont.cmin, cont.cmax)
Colorscale.extractScale(cont, {cLetter: 'c'})
);
}
}
Expand Down
6 changes: 1 addition & 5 deletions src/lib/gl_format_color.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,7 @@ function formatColor(containerIn, opacityIn, len) {

if(containerIn.colorscale !== undefined) {
sclFunc = Colorscale.makeColorScaleFunc(
Colorscale.extractScale(
containerIn.colorscale,
containerIn.cmin,
containerIn.cmax
)
Colorscale.extractScale(containerIn, {cLetter: 'c'})
);
}
else {
Expand Down
6 changes: 1 addition & 5 deletions src/traces/choropleth/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@ function styleTrace(gd, calcTrace) {
var markerLine = marker.line || {};

var sclFunc = Colorscale.makeColorScaleFunc(
Colorscale.extractScale(
trace.colorscale,
trace.zmin,
trace.zmax
)
Colorscale.extractScale(trace, {cLetter: 'z'})
);

locs.each(function(d) {
Expand Down
10 changes: 6 additions & 4 deletions src/traces/contour/make_color_map.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@ module.exports = function makeColorMap(trace) {
nc = 1;
}

var scl = trace.colorscale,
len = scl.length;
var scl = trace.reversescale ?
Colorscale.flipScale(trace.colorscale) :
trace.colorscale;

var domain = new Array(len),
range = new Array(len);
var len = scl.length;
var domain = new Array(len);
var range = new Array(len);

var si, i;

Expand Down
6 changes: 1 addition & 5 deletions src/traces/heatmap/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,7 @@ module.exports = function(gd, plotinfo, cdheatmaps, heatmapLayer) {
var context = canvas.getContext('2d');

var sclFunc = Colorscale.makeColorScaleFunc(
Colorscale.extractScale(
trace.colorscale,
trace.zmin,
trace.zmax
),
Colorscale.extractScale(trace, {cLetter: 'z'}),
{ noNumericCheck: true, returnArray: true }
);

Expand Down
2 changes: 1 addition & 1 deletion src/traces/scattermapbox/convert.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ function makeCircleOpts(calcTrace) {
if(arrayColor) {
if(Colorscale.hasColorscale(trace, 'marker')) {
colorFn = Colorscale.makeColorScaleFunc(
Colorscale.extractScale(marker.colorscale, marker.cmin, marker.cmax)
Colorscale.extractScale(marker, {cLetter: 'c'})
);
} else {
colorFn = Lib.identity;
Expand Down
Binary file modified test/image/baselines/gl3d_scatter-colorscale-marker.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
59 changes: 35 additions & 24 deletions test/jasmine/tests/colorscale_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -530,23 +530,6 @@ describe('Test colorscale:', function() {
expect(trace.autocolorscale).toBe(true);
expect(trace.colorscale).toEqual(colorscale);
});

it('should be reverse the auto scale when reversescale is true', function() {
trace = {
type: 'heatmap',
z: [['a', 'b'], [0.5, 'd']],
autocolorscale: true,
reversescale: true,
_input: {autocolorscale: true}
};
z = [[undefined, undefined], [0.5, undefined]];
gd = _supply(trace);
calcColorscale(gd, trace, {vals: z, containerStr: '', cLetter: 'z'});
expect(trace.autocolorscale).toBe(true);
expect(trace.colorscale[trace.colorscale.length - 1])
.toEqual([1, 'rgb(220,220,220)']);
});

});

describe('extractScale + makeColorScaleFunc', function() {
Expand All @@ -559,19 +542,47 @@ describe('Test colorscale:', function() {
[1, 'rgb(178,10,28)']
];

var specs = Colorscale.extractScale(scale, 2, 3);
var sclFunc = Colorscale.makeColorScaleFunc(specs);

it('should constrain color array values between cmin and cmax', function() {
var color1 = sclFunc(1),
color2 = sclFunc(2),
color3 = sclFunc(3),
color4 = sclFunc(4);
var trace = {
colorscale: scale,
pmin: 2,
pmax: 3
};

var specs = Colorscale.extractScale(trace, {cLetter: 'p'});
var sclFunc = Colorscale.makeColorScaleFunc(specs);

var color1 = sclFunc(1);
var color2 = sclFunc(2);
var color3 = sclFunc(3);
var color4 = sclFunc(4);

expect(color1).toEqual(color2);
expect(color1).toEqual('rgb(5, 10, 172)');
expect(color3).toEqual(color4);
expect(color4).toEqual('rgb(178, 10, 28)');
});

it('should flip color range when reversescale is true', function() {
var trace = {
colorscale: scale,
reversescale: true,
pmin: 2,
pmax: 3
};

var specs = Colorscale.extractScale(trace, {cLetter: 'p'});
var sclFunc = Colorscale.makeColorScaleFunc(specs);

var color1 = sclFunc(1);
var color2 = sclFunc(2);
var color3 = sclFunc(3);
var color4 = sclFunc(4);

expect(color1).toEqual(color2);
expect(color1).toEqual('rgb(178, 10, 28)');
expect(color3).toEqual(color4);
expect(color4).toEqual('rgb(5, 10, 172)');
});
});
});