Contour line labels #1815

alexcjohnson · 2017-06-23T21:32:18Z

Fixes #1395

TODO:

contourcarpet (cc @rreusser): currently the attributes are there (so we don't get errors) but they don't work. I haven't looked to see how easy it will be to add these there, if it's too hard we could just leave it for later (and remove the attributes and make supplyDefaults work without them)
tests (basic, labels without lines, panned, with formatting) - since this work is all in the plot step it'll probably be all in image tests
there's no configurability to how many labels you get, or where they go (there is style configurability though, for font, color, size, number format). I spent a good while on the location optimizer - which tries to minimize angle and maximize distance from other labels (especially those on the same level) and from the edge. This looks pleasing to my eye across all our test plots (and on zoom/pan), but I'd be curious @geocosmite if you see a use for more control in this regard.

cc @etpinard

no optimization at all yet

yeah, this needs major refactoring to be more useful for cases like this

rreusser · 2017-06-23T21:51:33Z

src/traces/contour/plot.js

-function makeLines(plotgroup, pathinfo, contours) {
+var TRAILING_ZEROS = /\.?0+$/;
+
+function makeLinesAndLabels(plotgroup, pathinfo, gd, cd0, contours, perimeter) {


Except for the innermost join content, it looks like mostly d3 operations so is probably okay, but this function comes out to 240 lines, which seems a bit on the high side.

rreusser · 2017-06-23T22:04:08Z

Very impressed with the greedy approach!

geocosmite · 2017-06-23T22:17:43Z

This looks FABULOUS! From these examples I don't think we would need more control on the positioning.

rreusser · 2017-06-23T22:19:29Z

src/traces/contour/plot.js

+
+                var maxLabels = Math.min(Math.ceil(pathLen / normLength),
+                    constants.LABELMAX);
+                var dp, p0, pMax, minCost, location, pMin;


Just a small note of caution that my syntax highlighter picks up location since window.location is special, though it's 100% fine here.

Ah good call, should be ok but I'll change it to avoid any confusion.

Only danger (I think) is if var declaration gets removed. Linter may or may not catch it. So really not dangerous at all… almost certainly 🔫

rreusser · 2017-06-24T01:57:57Z

Maybe a minimum path length cost component so that there aren't numbers on closed curves too small to display them? Otherwise I'm having trouble finding any problems with it. 🎉

alexcjohnson · 2017-06-26T16:15:30Z

@rreusser what plot did you see those labels on tiny loops? There is a minimum of 3x the label length but perhaps we need to increase that.

rreusser · 2017-06-26T16:18:25Z

@alexcjohnson zoomed in section on: http://rickyreusser.com/plotly-mock-viewer/#contour_transposed

Also, I've been thinking about carpet plots. Since plotting is pretty trace-specific, I think it might have to be duplicated, though it's probably possible to factor out the code since there's nothing really contour-specific. I'll try to throw together a POC (though probably not much work required beyond that).

alexcjohnson · 2017-06-26T18:13:00Z

@rreusser

Since plotting is pretty trace-specific, I think it might have to be duplicated, though it's probably possible to factor out the code since there's nothing really contour-specific. I'll try to throw together a POC (though probably not much work required beyond that).

awesome, that'll help with your previous comment too 🍰

using just width had some adverse effects with really short labels

alexcjohnson · 2017-06-26T18:43:17Z

src/traces/contour/plot.js

@@ -423,7 +423,7 @@ function makeLinesAndLabels(plotgroup, pathinfo, gd, cd0, contours, perimeter) {

                var isOpen = d3.select(this).classed('openline');

-                if(pathLen < textWidth * constants.LABELMIN) return;
+                if(pathLen < (textWidth + textHeight) * constants.LABELMIN) return;


I didn't increase LABELMIN, instead included both width and height of the label. That way when you have long labels you'll still get them on about the same length paths as before, but short labels will require a bit longer paths than they did previously:

Note that this isn't in the cost function, because that controls label location on the path but only weakly affects whether the path gets a label at all - ie if it pushes past MAXCOST

etpinard · 2017-06-26T20:29:28Z

src/traces/contourcarpet/attributes.js

-                'Has only an effect if `contours.coloring` is set to *fill*.'
-            ].join(' ')
-        },
+        showlines: contourContourAttrs.showlines,


🌴 thanks!

etpinard · 2017-06-26T20:47:49Z

src/traces/contour/attributes.js

+                'Determines whether to label the contour lines with their values.'
+            ].join(' ')
+        },
+        font: extendFlat({}, fontAttrs, {


I'd vote for labelfont here.

cc @cldougl

+1 for labelfont

-> labelFont in 2116a12

etpinard · 2017-06-28T02:04:18Z

src/plot_api/plot_schema.js

@@ -62,7 +64,11 @@ exports.get = function() {
    return {
        defs: {
            valObjects: Lib.valObjects,
-            metaKeys: UNDERSCORE_ATTRS.concat(['description', 'role'])
+            metaKeys: UNDERSCORE_ATTRS.concat(['description', 'role']),
+            editTypes: {


Ha. Sure, might as well list those things.

- so we might be able to reuse them for contourcarpet - and test getVisibleSegment

minor change to the algorithm probably due to cleaner open/closed criterion

this makes contours.coloring='lines' work with carpet also fixes some unusual bugs like if the first contour is not shown that were fixed long ago for contour

alexcjohnson · 2017-06-29T21:57:20Z

src/traces/contourcarpet/index.js

@@ -15,7 +15,7 @@ ContourCarpet.supplyDefaults = require('./defaults');
 ContourCarpet.colorbar = require('../contour/colorbar');
 ContourCarpet.calc = require('./calc');
 ContourCarpet.plot = require('./plot');
-ContourCarpet.style = require('./style');


continuing to 🌴 contour/contourcarpet... this also fixes some bugs such as:

contourcarpet failed with contours.coloring='lines'

if the first contour wasn't shown (if you give an explicit contour start value lower than what the data would be) the colors would have been wrong. For contour this was fixed in Contour bug #1309 (the key is to use d.level rather than i in coloring the contour lines/fills) and tested in contour_edge_cases.

@etpinard do you think it's sufficient to rely on the contour tests for contourcarpet too - since it's using the same code now - or would you like me to add contourcarpet tests of these cases?

do you think it's sufficient to rely

We have plenty of carpet mocks, maybe you could update one of them with contours.coloring='lines' ?

hmm, as far as I can tell only cheater_contour is not constraint-type, but perhaps I can extend that one to cover these cases. Sure.

0ff2dc4
This mock on master:

and now:

This is great. I'd have to dig in further to refresh my memory, but I struggled a bit to wrangle the behavior under control for both constraints and contours since it felt like they had slightly different semantics. Like constraints tend to be a single color while contours have multiple colors. But if it passes the existing image tests and fixes other behaviors, then I'm 100% content.

alexcjohnson · 2017-06-29T22:01:15Z

src/traces/contour/style.js


-        var colorMap = makeColorMap(trace);
+        // for contourcarpet only - is this a constraint-type contour trace?


A bit funny to have contourcarpet only code in contour.style - but perhaps someday we would like to support constraint-type coloring for regular contour plots too?

etpinard · 2017-06-29T22:20:06Z

src/traces/contourcarpet/index.js

@@ -15,7 +15,7 @@ ContourCarpet.supplyDefaults = require('./defaults');
 ContourCarpet.colorbar = require('../contour/colorbar');
 ContourCarpet.calc = require('./calc');
 ContourCarpet.plot = require('./plot');
-ContourCarpet.style = require('./style');
+ContourCarpet.style = require('../contour/style');


Amazing 🎉

alexcjohnson · 2017-06-30T00:35:40Z

src/traces/carpet/defaults.js

@@ -21,6 +21,8 @@ module.exports = function supplyDefaults(traceIn, traceOut, dfltColor, fullLayou
        return Lib.coerce(traceIn, traceOut, attributes, attr, dflt);
    }

+    traceOut._clipPathId = 'clip' + traceOut.uid + 'carpet';


@rreusser FYI - it turns out that we weren't applying the clip path to mocks that have the data trace before the carpet trace because the ID was cached during the plot step. I moved _clipPathId up to supplyDefaults to get around that. It's barely visible in the test images, but that's why they all changed (all the ones that put the data before the carpet anyway).

Also two more changes in here:

_clipPathId rather than clipPathId - if we're attaching something to the trace (or layout) that's not an attribute, it should either be a function or it should start with an underscore.

use Drawing.setClipUrl rather than directly setting the clip-path attribute. This is to handle pages with a <base> tag, which is obnoxious but important if it's present - which happens most notably in angular 😑

Oh wow… Good catch! Yeah, I can imagine that's a subtle half-border-width-effect. 👍

same as contour - it's *probably* ok without this but I'm a little worried about some restyle call changing element order unexpectedly.

also a bugfix to findBestTextLocation - surprised that didn't break anything before!

etpinard · 2017-06-30T17:42:48Z

src/traces/scattercarpet/plot.js

@@ -37,6 +38,6 @@ module.exports = function plot(gd, plotinfoproxy, data) {
        // separately to all scattercarpet traces, but that would require
        // lots of reorganization of scatter traces that is otherwise not
        // necessary. That makes this a potential optimization.
-        node.attr('clip-path', 'url(#clip' + carpet.uid + 'carpet)');
+        Drawing.setClipUrl(node, carpet._clipPathId);


Ha. That means carpet traces will now be functional in Angular 1.0 see #509

yeah, as mentioned in #1815 (comment)

Tried to figure out if we have a way to enforce that... only allow attr('clip-path', ...) inside Drawing.setClipUrl - like something to add to strict-d3? Seemed pretty invasive and hacky though.

etpinard · 2017-06-30T17:52:00Z

Amazing PR!

This will surely make the daunting contour code more-easily reusable (contourgeo anyone?).

💃

rreusser · 2017-06-30T17:28:59Z

src/components/annotations/draw.js

@@ -487,15 +487,15 @@ function drawRaw(gd, options, index, subplotId, xa, ya) {
            // to get the parity of the number of intersections.
            if(edges.reduce(function(a, x) {
                return a ^
-                    !!lineIntersect(headX, headY, headX + 1e6, headY + 1e6,
+                    !!Lib.segmentsIntersect(headX, headY, headX + 1e6, headY + 1e6,


Not part of this PR and I'm sure no meaningful performance hit, but is reduce necessary here? Seems like

for (i = 0; i < edges.length; i++) { if (!Lib.segmentsIntersect(...)) return; }

would be more concise and readable and would bail out early when it can.

Unless that equates to an all condition where you actually need to check all. Not 100% sure. The ^ is a bit confusing.

rreusser · 2017-06-30T17:29:21Z

src/components/annotations/draw.js

@@ -487,15 +487,15 @@ function drawRaw(gd, options, index, subplotId, xa, ya) {
            // to get the parity of the number of intersections.
            if(edges.reduce(function(a, x) {
                return a ^
-                    !!lineIntersect(headX, headY, headX + 1e6, headY + 1e6,
+                    !!Lib.segmentsIntersect(headX, headY, headX + 1e6, headY + 1e6,
                            x[0], x[1], x[2], x[3]);
            }, false)) {
                // no line or arrow - so quit drawArrow now
                return;
            }

            edges.forEach(function(x) {


Is forEach avoidable here?

rreusser · 2017-06-30T17:33:42Z

src/components/drawing/index.js

+ *   for this element beforehand.
+ *
+ * @return {object}: a plain object containing the width, height, left, right,
+ *   top, and bottom of `node`


❤️ 📚

rreusser · 2017-06-30T17:35:47Z

src/components/drawing/index.js

 */
 drawing.savedBBoxes = {};
 var savedBBoxesCount = 0;
 var maxSavedBBoxes = 10000;

-drawing.bBox = function(node, hash) {
+drawing.bBox = function(node, inTester, hash) {


👍 technically an internal API used elsewhere, but a quick grep I can't find any cases where the argument signature change is invalid.

rreusser · 2017-06-30T17:40:33Z

src/lib/geometry2d.js

+    // ignore the case where they are colinear
+    if(det === 0) return null;
+    var t = (b * f - c * e) / det,
+        u = (b * d - a * e) / det;


If I were really going to nitpick, I'd suggest comparing bounding boxes first and here multiplying through by det to eliminate division, but this is probably not a bottleneck…

rreusser · 2017-06-30T17:46:53Z

src/traces/contour/attributes.js

@@ -36,6 +37,7 @@ module.exports = extendFlat({}, {
        valType: 'boolean',
        dflt: true,
        role: 'style',
+        editType: 'docalc',


😮

Now I understand. Nice.

rreusser · 2017-06-30T17:48:44Z

src/traces/contour/plot.js



-module.exports = function plot(gd, plotinfo, cdcontours) {
+exports.plot = function plot(gd, plotinfo, cdcontours) {


Named for testing?

Reuse in carpet. Got it.

rreusser · 2017-06-30T17:51:47Z

src/traces/contour/plot.js

+    return lineClip;
+};
+
+exports.labelFormatter = function(contours, colorbar, fullLayout) {


Interesting. Does this have any more general applicability? I feel like piggybacking on this is a common need, and I struggled to figure out just which parts and in what sequence it should be done.

rreusser · 2017-06-30T17:52:29Z

src/traces/contour/plot.js

+    };
+};
+
+exports.findBestTextLocation = function(path, pathBounds, textOpts, labelData, plotBounds) {


❤️ ❤️ ❤️ Reusable for carpet?

rreusser · 2017-06-30T17:56:53Z

src/traces/contourcarpet/index.js

@@ -15,7 +15,7 @@ ContourCarpet.supplyDefaults = require('./defaults');
 ContourCarpet.colorbar = require('../contour/colorbar');
 ContourCarpet.calc = require('./calc');
 ContourCarpet.plot = require('./plot');
-ContourCarpet.style = require('./style');


This is great. I'd have to dig in further to refresh my memory, but I struggled a bit to wrangle the behavior under control for both constraints and contours since it felt like they had slightly different semantics. Like constraints tend to be a single color while contours have multiple colors. But if it passes the existing image tests and fixes other behaviors, then I'm 100% content.

rreusser · 2017-06-30T18:07:14Z

Looks great to me! Thrilled for carpet to get a close second look. One nitpick is that -5.0 and 5.0 are sometimes hard to distinguish since the - tends to blend into the surrounding contour. But I don't really see how we can fix that, unless the label padding were configurable so that you could add a bit more spacing.

💃

alexcjohnson · 2017-06-30T18:12:26Z

One nitpick is that -5.0 and 5.0 are sometimes hard to distinguish since the - tends to blend into the surrounding contour.

Hmm, interesting - I suppose we could either add some extra space ahead of a leading -, or replace it with a different unicode symbol that's raised up a bit, perhaps? I'll leave it as is for the first iteration but will take a look at it later.

etpinard · 2017-06-30T18:15:47Z

Hmm, interesting - I suppose we could either add some extra space ahead of a leading -, or replace it with a different unicode symbol that's raised up a bit, perhaps? I'll leave it as is for the first iteration but will take a look at it later.

... or implement #1597

rreusser · 2017-06-30T18:40:04Z

This is the only other variation that jumps out. Which I don't love. Or +1/-1 instead of 1/-1, which I don't like either. I think the text shadow method seems best.

alexcjohnson added 13 commits June 21, 2017 15:23

put all contour lines into one group (so we can clip it later)

468ef5d

collect contour clipPath in the main <defs>

88cc33c

more standard format for contour constants

ee7a839

move lineIntersect -> Lib.geometry2d

b56f4ea

segmentDistance and tests of geometry2d

d534fb2

contour line label basic functionality

d1e3448

no optimization at all yet

better test of alternate Drawing.font syntax

b8c75c0

use contour/attributes in contourcarpet/attributes

2f3a712

pull path measurement routines out into Lib

6299303

allow contour labels even without lines

99f39f8

simple contour label position optimization

d518c83

use axis formatting for contour labels

27f8cfe

yeah, this needs major refactoring to be more useful for cases like this

include label attrs in contourcarpet to avoid errors but they're noops

221b124

alexcjohnson added status: in progress feature something new labels Jun 23, 2017

rreusser reviewed Jun 23, 2017

View reviewed changes

etpinard added this to the 1.29.0 milestone Jun 26, 2017

location -> loc (window global potential confusion)

ca1cced

alexcjohnson added 2 commits June 26, 2017 14:21

scale min path length for contour labels to width + height

d52cfba

using just width had some adverse effects with really short labels

🐄 remove obsolete regexp

0f37648

alexcjohnson commented Jun 26, 2017

View reviewed changes

etpinard reviewed Jun 26, 2017

View reviewed changes

etpinard removed the status: in progress label Jun 28, 2017

etpinard reviewed Jun 28, 2017

View reviewed changes

alexcjohnson added 3 commits June 29, 2017 00:48

refactor contour/plot for smaller functions

fb4b690

- so we might be able to reuse them for contourcarpet - and test getVisibleSegment

updated contour_edge_cases baseline

b0f9bb2

minor change to the algorithm probably due to cleaner open/closed criterion

unify contour.style and contourcarpet.style

e1caa9e

this makes contours.coloring='lines' work with carpet also fixes some unusual bugs like if the first contour is not shown that were fixed long ago for contour

alexcjohnson commented Jun 29, 2017

View reviewed changes

etpinard reviewed Jun 29, 2017

View reviewed changes

alexcjohnson added 5 commits June 29, 2017 18:42

cover contour edge cases with contourcarpet

0ff2dc4

fix contour style when coloring=none

f5873f1

a little more stuff contourcarpet can inherit from contour

b5a674f

fix carpet plot clip path

1f85a6f

🔪 some obsolete code in carpet/plot

6c2043e

alexcjohnson commented Jun 30, 2017

View reviewed changes

alexcjohnson added 4 commits June 29, 2017 20:36

update histogram2dcontour for contour.plot API change

bbb9574

nest scattercontour lines inside a contourlines group

9495dee

same as contour - it's *probably* ok without this but I'm a little worried about some restyle call changing element order unexpectedly.

fixup font->labelfont in contourcarpet

7f086e0

labels on contourcarpet

49b8141

also a bugfix to findBestTextLocation - surprised that didn't break anything before!

alexcjohnson changed the title ~~[WIP] Contour line labels~~ Contour line labels Jun 30, 2017

alexcjohnson added 2 commits June 30, 2017 13:18

update comment on contourcarpet label bounds

8934038

update baselines for findBestTextLocation bugfix

dbeecd0

etpinard reviewed Jun 30, 2017

View reviewed changes

rreusser approved these changes Jun 30, 2017

View reviewed changes

alexcjohnson merged commit fbc0296 into master Jun 30, 2017

alexcjohnson deleted the contour-labels branch June 30, 2017 18:16


		var colorMap = makeColorMap(trace);
		// for contourcarpet only - is this a constraint-type contour trace?



		module.exports = function plot(gd, plotinfo, cdcontours) {
		exports.plot = function plot(gd, plotinfo, cdcontours) {

Contour line labels #1815

Contour line labels #1815

Conversation

alexcjohnson commented Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

rreusser commented Jun 23, 2017

geocosmite commented Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

rreusser commented Jun 24, 2017

alexcjohnson commented Jun 26, 2017

rreusser commented Jun 26, 2017

alexcjohnson commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented Jun 30, 2017

alexcjohnson commented Jun 30, 2017

etpinard commented Jun 30, 2017

rreusser commented Jun 30, 2017

alexcjohnson commented Jun 23, 2017 •

edited

Loading

geocosmite commented Jun 23, 2017 •

edited

Loading

rreusser Jun 26, 2017 •

edited

Loading