Skip to content

Contour bg fix #1280

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.

Sign up for GitHub

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 3 commits into from
Jan 4, 2017
Merged

Contour bg fix #1280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bfa30db
handle falsey values correctly in coerce2
alexcjohnson Jan 4, 2017
cef9e66
fix trace.contours edge cases and relayout bug with autocontour
alexcjohnson Jan 4, 2017
317d0c7
Merge branch 'master' into contour-bg-fix
alexcjohnson Jan 4, 2017
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
5 changes: 3 additions & 2 deletions src/lib/coerce.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,9 +315,10 @@ exports.coerce = function(containerIn, containerOut, attributes, attribute, dflt
*/
exports.coerce2 = function(containerIn, containerOut, attributes, attribute, dflt) {
var propIn = nestedProperty(containerIn, attribute),
propOut = exports.coerce(containerIn, containerOut, attributes, attribute, dflt);
propOut = exports.coerce(containerIn, containerOut, attributes, attribute, dflt),
valIn = propIn.get();

return propIn.get() ? propOut : false;
return (valIn !== undefined && valIn !== null) ? propOut : false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@etpinard I included this change bcs contours uses coerce2 on some numeric values that can be 0. This still seems a little weird, ie you have to know to check for exactly false, and if false is an allowed value it still won't be telling you what you think it is. But this fixes it for the exact case I was worried about here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

I've been wanting to merge coerce2 with validate for a while now.

Good enough for now 👍

};

/*
Expand Down
22 changes: 17 additions & 5 deletions src/traces/contour/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,15 @@ module.exports = extendFlat({}, {
},
ncontours: {
valType: 'integer',
dflt: 0,
dflt: 15,
min: 1,
role: 'style',
description: [
'Sets the maximum number of contour levels. The actual number',
'of contours will be chosen automatically to be less than or',
'equal to the value of `ncontours`.',
'Has an effect only if `autocontour` is *true*.'
'Has an effect only if `autocontour` is *true* or if',
'`contours.size` is missing.'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So if you specify ncontours and contours: {start, end} we will automatically choose a "nice" size, even though autocontour is false. Any reason not to allow this usage?

Note that the converse is not allowed: you can't specify autocontour=true and use your own contours.size value instead of ncontours - though I suppose it would be clear what this means, as it uses the same machinery as axis ticks, ie it would use a canonical tick of 0 so the contours would be all the multiples of contours.size between the data min and max values. That would actually be kind of nice, maybe I'll add it as well.

Copy link
Contributor

@etpinard etpinard Jan 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Any reason not to allow this usage?

I can't think of one. Nice addition 👍

Note that the converse is not allowed:

So,

autocontour: true,
contours: {
  size: 10
}

ignores contours.size, and

autocontour: false,
contours: {
  size: 10
}

pick contours.start and contours.end using Axes.tickFirst?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, I was starting to think autocontour had become completely obsolete, we’d just fill in whatever missing pieces there were. But actually I think I want to make some changes to this:

  • with autocontour=true we clear start, end, and size and generate them all automatically from the data range and ncontours. Perhaps the only reason you'd need to do this is if you previously set some non-auto values and you want to toss them out.
  • with autocontour=false we still fill in anything that’s missing but won’t ignore anything the user provides. So for example if you only want to specify contours.end to ignore some outliers, you can do that, you don’t have to also specify contours.start to get it accepted.

I don't think there's anything backward-incompatible in this proposal, except in partial information cases, where we had a bunch of bugs anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deferred to #1282

].join(' ')
},

Expand All @@ -59,19 +61,29 @@ module.exports = extendFlat({}, {
valType: 'number',
dflt: null,
role: 'style',
description: 'Sets the starting contour level value.'
description: [
'Sets the starting contour level value.',
'Must be less than `contours.end`'
].join(' ')
},
end: {
valType: 'number',
dflt: null,
role: 'style',
description: 'Sets the end contour level value.'
description: [
'Sets the end contour level value.',
'Must be more than `contours.start`'
].join(' ')
},
size: {
valType: 'number',
dflt: null,
min: 0,
role: 'style',
description: 'Sets the step between each contour level.'
description: [
'Sets the step between each contour level.',
'Must be positive.'
].join(' ')
},
coloring: {
valType: 'enumerated',
Expand Down
67 changes: 55 additions & 12 deletions src/traces/contour/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
'use strict';

var Axes = require('../../plots/cartesian/axes');
var extendFlat = require('../../lib').extendFlat;
var heatmapCalc = require('../heatmap/calc');


Expand All @@ -22,30 +23,72 @@ module.exports = function calc(gd, trace) {

// check if we need to auto-choose contour levels
if(trace.autocontour !== false) {
var dummyAx = {
type: 'linear',
range: [trace.zmin, trace.zmax]
};
var dummyAx = autoContours(trace.zmin, trace.zmax, trace.ncontours);

Axes.autoTicks(
dummyAx,
(trace.zmax - trace.zmin) / (trace.ncontours || 15)
);
contours.size = dummyAx.dtick;

contours.start = Axes.tickFirst(dummyAx);
contours.size = dummyAx.dtick;
dummyAx.range.reverse();
contours.end = Axes.tickFirst(dummyAx);

if(contours.start === trace.zmin) contours.start += contours.size;
if(contours.end === trace.zmax) contours.end -= contours.size;

// so rounding errors don't cause us to miss the last contour
contours.end += contours.size / 100;
// if you set a small ncontours, *and* the ends are exactly on zmin/zmax
// there's an edge case where start > end now. Make sure there's at least
// one meaningful contour, put it midway between the crossed values
if(contours.start > contours.end) {
contours.start = contours.end = (contours.start + contours.end) / 2;
}

// copy auto-contour info back to the source data.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is actually already dealt with in contour.plot, which is easier because we only have to do it once, rather than separately for every edge case we're now going to be checking for here.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch 👍

trace._input.contours = contours;
trace._input.contours = extendFlat({}, contours);
}
else {
// sanity checks on manually-supplied start/end/size
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good 👍

var start = contours.start,
end = contours.end,
inputContours = trace._input.contours;

if(start > end) {
contours.start = inputContours.start = end;
end = contours.end = inputContours.end = start;
start = contours.start;
}

if(!(contours.size > 0)) {
var sizeOut;
if(start === end) sizeOut = 1;
else sizeOut = autoContours(start, end, trace.ncontours).dtick;

inputContours.size = contours.size = sizeOut;
}
}

return cd;
};

/*
* autoContours: make a dummy axis object with dtick we can use
* as contours.size, and if needed we can use Axes.tickFirst
* with this axis object to calculate the start and end too
*
* start: the value to start the contours at
* end: the value to end at (must be > start)
* ncontours: max number of contours to make, like roughDTick
*
* returns: an axis object
*/
function autoContours(start, end, ncontours) {
var dummyAx = {
type: 'linear',
range: [start, end]
};

Axes.autoTicks(
dummyAx,
(end - start) / (ncontours || 15)
);

return dummyAx;
}
15 changes: 12 additions & 3 deletions src/traces/contour/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,19 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout

var contourStart = Lib.coerce2(traceIn, traceOut, attributes, 'contours.start'),
contourEnd = Lib.coerce2(traceIn, traceOut, attributes, 'contours.end'),
autocontour = coerce('autocontour', !(contourStart && contourEnd));
missingEnd = (contourStart === false) || (contourEnd === false),

if(autocontour) coerce('ncontours');
else coerce('contours.size');
// normally we only need size if autocontour is off. But contour.calc
// pushes its calculated contour size back to the input trace, so for
// things like restyle that can call supplyDefaults without calc
// after the initial draw, we can just reuse the previous calculation
contourSize = coerce('contours.size'),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line - always coercing contours.size rather than only if autocontour is false - is actually all you need to fix #591

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

autoContour;

if(missingEnd) autoContour = traceOut.autocontour = true;
else autoContour = coerce('autocontour', false);

if(autoContour || !contourSize) coerce('ncontours');

handleStyleDefaults(traceIn, traceOut, coerce, layout);
};
8 changes: 7 additions & 1 deletion src/traces/contour/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,9 @@ function plotOne(gd, plotinfo, cd) {
}

function emptyPathinfo(contours, plotinfo, cd0) {
var cs = contours.size || 1,
var cs = contours.size,
pathinfo = [];

for(var ci = contours.start; ci < contours.end + cs / 10; ci += cs) {
pathinfo.push({
level: ci,
Expand All @@ -103,6 +104,11 @@ function emptyPathinfo(contours, plotinfo, cd0) {
z: cd0.z,
smoothing: cd0.trace.line.smoothing
});

if(pathinfo.length > 1000) {
Lib.warn('Too many contours, clipping at 1000', contours);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

somewhat arbitrary... but consistent with what we do for cartesian ticks - here I included the warning as the consequences (in terms of super slow perf) are likely much more severe and you will want to see the contours object to know why it happened.

break;
}
}
return pathinfo;
}
Expand Down
77 changes: 73 additions & 4 deletions test/jasmine/tests/contour_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ describe('contour defaults', function() {
[0.625, 1.25, 3.125, 6.25]],
contours: {
start: 4,
end: 14,
size: 0.5
end: 14
// missing size does NOT set autocontour true
// even though in calc we set an autosize.
}
};
supplyDefaults(traceIn, traceOut, defaultColor, layout);
Expand All @@ -46,7 +47,8 @@ describe('contour defaults', function() {
z: [[10, 10.625, 12.5, 15.625],
[5.625, 6.25, 8.125, 11.25],
[2.5, 3.125, 5.0, 8.125],
[0.625, 1.25, 3.125, 6.25]]
[0.625, 1.25, 3.125, 6.25]],
contours: {start: 4} // you need at least start and end
};
supplyDefaults(traceIn, traceOut, defaultColor, layout);
expect(traceOut.autocontour).toBe(true);
Expand Down Expand Up @@ -183,7 +185,9 @@ describe('contour calc', function() {
Plots.supplyDefaults(gd);
var fullTrace = gd._fullData[0];

return Contour.calc(gd, fullTrace)[0];
var out = Contour.calc(gd, fullTrace)[0];
out.trace = fullTrace;
return out;
}

it('should fill in bricks if x/y not given', function() {
Expand Down Expand Up @@ -269,4 +273,69 @@ describe('contour calc', function() {
expect(out.y).toBeCloseToArray([0, 1]);
expect(out.z).toBeCloseTo2DArray([[1, 2, 3], [3, 1, 2]]);
});

it('should make nice autocontour values', function() {
var incompleteContours = [
undefined,
{start: 12},
{end: 45},
{start: 2, size: 2} // size gets ignored
];

var contoursFinal = [
// fully auto. These are *not* exactly the output contours objects,
// I put the input ncontours in here too.
{inputNcontours: undefined, start: 0.5, end: 4.5, size: 0.5},
// explicit ncontours
{inputNcontours: 6, start: 1, end: 4, size: 1},
// edge case where low ncontours makes start and end cross
{inputNcontours: 2, start: 2.5, end: 2.5, size: 5}
];

incompleteContours.forEach(function(contoursIn) {
contoursFinal.forEach(function(spec) {
var out = _calc({
z: [[0, 2], [3, 5]],
contours: contoursIn,
ncontours: spec.inputNcontours
}).trace;

['start', 'end', 'size'].forEach(function(attr) {
expect(out.contours[attr]).toBe(spec[attr], [contoursIn, attr]);
// all these get copied back to the input trace
expect(out._input.contours[attr]).toBe(spec[attr], [contoursIn, attr]);
});
});
});
});

it('should supply size and reorder start/end if autocontour is off', function() {
var specs = [
{start: 1, end: 100, ncontours: undefined, size: 10},
{start: 1, end: 100, ncontours: 5, size: 20},
{start: 10, end: 10, ncontours: 10, size: 1}
];

specs.forEach(function(spec) {
[
[spec.start, spec.end, 'normal'],
[spec.end, spec.start, 'reversed']
].forEach(function(v) {
var startIn = v[0],
endIn = v[1],
order = v[2];

var out = _calc({
z: [[1, 2], [3, 4]],
contours: {start: startIn, end: endIn},
ncontours: spec.ncontours
}).trace;

['start', 'end', 'size'].forEach(function(attr) {
expect(out.contours[attr]).toBe(spec[attr], [spec, order, attr]);
expect(out._input.contours[attr]).toBe(spec[attr], [spec, order, attr]);
});
});
});
});
});
4 changes: 2 additions & 2 deletions test/jasmine/tests/lib_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ describe('Test lib.js:', function() {

it('should set a value and return the value it sets when user input is valid', function() {
var colVal = 'red',
sizeVal = 14,
sizeVal = 0, // 0 is valid but falsey
attrs = {testMarker: {testColor: {valType: 'color', dflt: 'rgba(0, 0, 0, 0)'},
testSize: {valType: 'number', dflt: 20}}},
obj = {testMarker: {testColor: colVal, testSize: sizeVal}},
Expand Down Expand Up @@ -706,7 +706,7 @@ describe('Test lib.js:', function() {

it('should return false if there is no user input', function() {
var colVal = null,
sizeVal = null,
sizeVal, // undefined
attrs = {testMarker: {testColor: {valType: 'color', dflt: 'rgba(0, 0, 0, 0)'},
testSize: {valType: 'number', dflt: 20}}},
obj = {testMarker: {testColor: colVal, testSize: sizeVal}},
Expand Down