Merge pull request #1280 from plotly/contour-bg-fix

Contour bg fix

Author: alexcjohnson

src/lib/coerce.js

@@ -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;
 };
 
 /*

src/traces/contour/attributes.js

@@ -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.'
         ].join(' ')
     },
 
@@ -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',

src/traces/contour/calc.js

@@ -10,6 +10,7 @@
 'use strict';
 
 var Axes = require('../../plots/cartesian/axes');
+var extendFlat = require('../../lib').extendFlat;
 var heatmapCalc = require('../heatmap/calc');
 
 
@@ -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.
-        trace._input.contours = contours;
+        trace._input.contours = extendFlat({}, contours);
+    }
+    else {
+        // sanity checks on manually-supplied start/end/size
+        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;
+}

src/traces/contour/defaults.js

@@ -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'),
+        autoContour;
+
+    if(missingEnd) autoContour = traceOut.autocontour = true;
+    else autoContour = coerce('autocontour', false);
+
+    if(autoContour || !contourSize) coerce('ncontours');
 
     handleStyleDefaults(traceIn, traceOut, coerce, layout);
 };

src/traces/contour/plot.js

@@ -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,
@@ -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);
+            break;
+        }
     }
     return pathinfo;
 }

test/jasmine/tests/contour_test.js

@@ -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);
@@ -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);
@@ -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() {
@@ -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]);
+                });
+            });
+        });
+    });
 });

test/jasmine/tests/lib_test.js

@@ -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}},
@@ -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}},