Implement attributes base, offset and width in bar traces (issue 80) #3

n-riesco · 2016-10-21T10:47:19Z

This PR implements attributes base, offset and width as discussed in plotly#80 .

I've tested the PR mostly by playing with mock images, and I'm yet to commit the jasmine tests.

Although I've been careful to maintain the original behaviour of all barmodes, I agree with Étienne, here, that more jasmine tests are needed to avoid any regressions.

@etpinard Are there any cases in particular that you'd like to see tested?

* Simplified algorithm to identify overlapping bars. * Removed the closure setBarCenter. * Checked that jasmine tests still pass.

* Converted the closure that groups vertical and horizontal bars into a function. * This change will help split setPositions into functions for each barmode.

* Moved code to group bars from setGroupPositions to setPositions.

* Converted closure `barpositions` into function `setOffsetAndWidth`. * This change will help split setPositions into functions for each barmode.

* Don't assume the position axis is `xa` in the calculation of `barDelta` in `bar/hover.js`. * Updated `tests/bar_test.js` to account for the replacement of `t.bar` with `t.bargroupwidth`. * Updated the group case in `tests/bar_test.js` to test the use of `layout.bargroupgap`.

* Refactored the code to set bar positions in overlay mode into function `setGroupPositionsInOverlayMode`.

* Refactored the code for setting bar positions into setGroupPositionsInOverlayMode, setGroupPositionsInGroupMode and setGroupPositionsInStackOrRelativeMode. * Refactored code for stacking bars and computing minDiff into helper class Sieve.

* Renamed setBaseAndSize to stackBars. * Refactor code to normalize bars into function normalizeBars.

* Allow minDtick update if barmode is group and bars overlap.

* Function sieveBars sieves all the bars without updating the bar bases. This step is required before calling normalizeBars. * Function stackBars sieves all the bars and updates the bar bases and tops accordingly.

etpinard · 2016-10-21T14:13:52Z

Are there any cases in particular that you'd like to see tested?

Making sure that the bar calc / set-position steps have 100% test coverage would be a great start. Unfortunately, we haven't hooked in a test coverage tool in our jasmine runner yet, so this process will be somewhat manual. My apologies.

Thanks again for making a PR onto your fork first. 🎉 This should make the commit history cleaner in the main plotly.js repo. I'll give your PR a first pass now.

etpinard · 2016-10-21T14:26:59Z

src/traces/bar/calc.js

+    // set size
+    for(i = 0; i < cd.length; i++) {
+        if(isNumeric(size[i])) {
+            cd[i].s = size[i] - cd[i].b;


⛔ We'll have to guard against non-numeric base items when base is set to an array.

In brief, in the supply-defaults step, scalar base inputs are coerce to numbers, but the supply-defaults step is designed to never penetrate array values, so non-numeric array items have to be handled in the calc step.

Oh and by the way, half-valid array inputs are an example of test cases that should included in our jasmine suites.

etpinard · 2016-10-21T14:27:48Z

src/traces/bar/hover.js

@@ -21,7 +21,8 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
        xa = pointData.xa,
        ya = pointData.ya,
        barDelta = (hovermode === 'closest') ?
-            t.barwidth / 2 : t.dbar * (1 - xa._gd._fullLayout.bargap) / 2,
+            t.barwidth / 2 :
+            t.bargroupwidth,


nice simplification. 👍

etpinard · 2016-10-21T14:31:30Z

src/traces/bar/plot.js

+                        x0 = xa.c2p(p0, true);
+                        x1 = xa.c2p(p1, true);
+                        y0 = ya.c2p(s0, true);
+                        y1 = ya.c2p(s1, true);


very clean here. 🍻

etpinard · 2016-10-21T14:39:21Z

src/traces/bar/set_positions.js

+        dontMergeOverlappingData = !barnorm;
+
+    // update position axis and set bar offsets and widths
+    calcTraces.forEach(function(calcTrace) {


let's rewrite this as a for-loop for speed 🐎

etpinard · 2016-10-21T14:51:31Z

test/image/mocks/bar_attrs_group_norm.json

+    "data":[
+        {
+            "base":6,
+            "y":[1,2,3,4],


Oof. That took me a while to understand what's going on here. But, I think you got it right.

base always refers to the start of the bars and y always refers to their end positions - even though the bar base can be above the bar y coordinate.

wait, doesn't y normally refer to the bar's size, not its end position?

Yeah, I really think it should be size. y is the primary data "value" attribute (for vertical bars) and for a bar chart, the bar's value is its size, where it starts or ends is a secondary level of analysis.

@alexcjohnson ~~I think making y mean size would be confusing, because I'd expect the values in y and the ticks in yaxis to be the same.~~

I think you're right. The thing that made me think of y as end positions was that we store the end position in calcdata[i].y.

etpinard · 2016-10-21T15:07:17Z

src/traces/bar/attributes.js

+        valType: 'any',
+        arrayOk: true,
+        role: 'info',
+        description: [


To make the fact that these attributes are optional, we usually set the dflt: null (e.g. contour trace start, end, size).

Note that, null are ignored upon Lib.nestedProperty(/**/).set(), so your fullTrace.base === undefined will still work.

etpinard · 2016-10-21T15:07:34Z

src/traces/bar/set_positions.js

+            calcTrace = calcTraces[i];
+            fullTrace = calcTrace[0].trace;
+
+            if(fullTrace.offset === undefined) included.push(calcTrace);


nice pattern 👍

etpinard · 2016-10-21T15:18:30Z

Thanks again @n-riesco for making the bar set-position step very nicely readable.

Apart from my comment about guarding against non-valid base, offset and width array items, I didn't catch any 🔴 flags.

So, before making a PR to the main plotly.js repo:

resolve @alexcjohnson 's Implement attributes base, offset and width in bar traces (issue 80) #3 (comment)
add dflt: null in the attribute declaration Implement attributes base, offset and width in bar traces (issue 80) #3 (comment)
speed improvement Implement attributes base, offset and width in bar traces (issue 80) #3 (comment)
make sure to that our bar jasmine suite has test cases that cover all setXXXinYYYmode functions of the revamped setPositions.

alexcjohnson · 2016-10-21T15:24:11Z

(for vertical bars)

reminds me we should have one test image with horizontal bars and these attributes.

* Null defaults are used to denote optional properties. * Note that null values are ignored by `Lib.nestedProperty(/**/).set()`, so that the pattern `fullTrace.base === undefined` still works.

* Added note to explain the reason why setGroupPositionsInOverlayMode handles the case barnorm is defined.

* Commit bar tests before implementing the commit that interprets trace data as sizes. This should help ensure the implementation is thorough.

* Updated bar tests that set the base attribute, so that now trace data is interpreted as sizes.

* Update bar_attrs_* mock images to account for trace data being interpreted as sizes. * bar_attrs_group.json: updated y to store sizes instead of bar tops. * bar_attrs_group_norm.json: updated base and y, so that it's clear that this mock tests the use of base and normalisation. * bar_attrs_overlay.json: updated y to store sizes instead of bar tops.

* Converted bar_attrs_group_norm into a horizontal bar plot.

* Updated baseline, because now this mock is a horizontal bar plot.

n-riesco · 2016-10-24T09:57:45Z

@etpinard @alexcjohnson I've addressed all the feedback. This PR is ready for review again.

etpinard · 2016-10-24T14:11:19Z

src/traces/bar/set_positions.js

+        //
+        // (note that `setGroupPositionsInOverlayMode` handles the case barnorm
+        // is defined, because this function is also invoked for traces that
+        // can't be grouped or stacked)


alexcjohnson · 2016-10-24T14:16:11Z

💃 from my side - thanks!

etpinard · 2016-10-24T14:21:17Z

test/jasmine/tests/bar_test.js

+    });
+});
+
+describe('Bar.calc', function() {


Very nice.

Thanks very much!

etpinard · 2016-10-24T14:23:54Z

src/traces/bar/set_positions.js

@@ -348,6 +358,11 @@ function setOffsetAndWidthInGroupMode(gd, pa, sieve) {
            // if width is an array, then clone it into t.barwidth.
            var newBarwidth = width.slice(0, calcTrace.length);

+            // guard against non-numeric items
+            for(j = 0; j < newBarwidth.length; j++) {


@n-riesco is there a way to place this for-loop in a place common to setOffsetAndWidthInGroupMode and setOffsetAndWidth ?

Good catch! Since offset is undefined for all the traces in group mode, both functions can share the same implementation.

etpinard · 2016-10-24T14:32:11Z

src/traces/bar/set_positions.js

-        vpad = pMaxWidth / 2;
-        Axes.expand(pa, [pMax + pMaxOffset + vpad], {vpad: vpad});
-    }
+    Axes.expand(pa, [pMin, pMax], {padded: false});


@n-riesco would you mind sharing some info here about why this patch was needed? Thanks.

Before having offset and width, the range of the position axis was computed using the position data x padded with vpad = minDiff/2 (minDiff being the maximum size of a bar group).

With offset and width, bars can be shifted away from their positions x and widths can be larger than minDiff. Here I'm iterating each bar and computing their left and right sides (taking into account offset and width). This helps me compute the pMin and pMax needed to expand the position axis, so that all bars are fully within the range of the position axis.

I will add a comment in the code to explain this.

etpinard · 2016-10-24T14:32:47Z

src/traces/bar/set_positions.js

@@ -566,7 +566,7 @@ function normalizeBars(gd, sa, sieve) {

            if(!isNumeric(bar.s)) continue;

-            var scale = Math.abs(sTop / sieve.get(bar.p, bar.b + bar.s));
+            var scale = Math.abs(sTop / sieve.get(bar.p, bar.s));


glad to see if was that easy to make the switch 👍

etpinard · 2016-10-24T14:36:58Z

test/image/mocks/bar_attrs_group_norm.json

+            "type":"bar"
+        }, {
+            "base":[7,6,5,4],
+            "x":[1,2,3,4],


very nice mock!

etpinard · 2016-10-24T14:42:44Z

test/image/mocks/bar_attrs_overlay.json

+        }, {
+            "base":[0,1,3,2],
+            "width":1,
+            "offset":[1.5],


very nice here, offset[0] is only applied to the first bar.

etpinard · 2016-10-24T14:51:27Z

Awesome @n-riesco 👍

My two comments #3 (comment) and #3 (comment) are non-blocking.

After rebasing the test commits into one for all mock / baselines additions and one for all jasmine updates / additions, PR away to main repo!

alexcjohnson · 2017-08-31T14:21:38Z

@AdnanBoota we got the message, it's not helpful to post this question more than once.

n-riesco added 19 commits October 17, 2016 15:15

bar: use descriptive variable names

03e5072

* Simplified algorithm to identify overlapping bars. * Removed the closure setBarCenter. * Checked that jasmine tests still pass.

bar: convert closure into function

bbdc385

* Converted the closure that groups vertical and horizontal bars into a function. * This change will help split setPositions into functions for each barmode.

bar: refactor code to group bars

7b14fcd

* Moved code to group bars from setGroupPositions to setPositions.

bar: rename barpositions to setOffsetAndWidth

fe66e3c

* Converted closure `barpositions` into function `setOffsetAndWidth`. * This change will help split setPositions into functions for each barmode.

bar: refactor code into function setBaseAndSize

d422f67

bar: Move code for overlay mode into a function

93ca66e

* Refactored the code to set bar positions in overlay mode into function `setGroupPositionsInOverlayMode`.

bar: use Sieve to stack bars in setBaseAndSize

3b87e85

bar: add functions stackBars and normalizeBars

a7ca755

* Renamed setBaseAndSize to stackBars. * Refactor code to normalize bars into function normalizeBars.

bar: fix image test failure in hist_grouped

1400311

* Allow minDtick update if barmode is group and bars overlap.

bar: do not normalise bars with no size

4294cad

bar: Add attributes base, offset and width

4866cda

bar: add function sieveBars

79f2e8d

* Function sieveBars sieves all the bars without updating the bar bases. This step is required before calling normalizeBars. * Function stackBars sieves all the bars and updates the bar bases and tops accordingly.

bar: do not assume initial bar base is zero

10b678b

bar: ensure size axis includes bar bases

23f7415

bar: do not stack traces that set bar base

6f05b34

bar: implement attributes base, offset and width

d4f7747

test: add bar charts using base, offset and width

0e6e0f1

etpinard reviewed Oct 21, 2016

View reviewed changes

src/traces/bar/plot.js

x0 = xa.c2p(p0, true);

x1 = xa.c2p(p1, true);

y0 = ya.c2p(s0, true);

y1 = ya.c2p(s1, true);

Copy link

etpinard Oct 21, 2016

Choose a reason for hiding this comment

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

very clean here. 🍻

etpinard reviewed Oct 21, 2016

View reviewed changes

bar: guard against invalid base items

e83f4c5

n-riesco added 11 commits October 21, 2016 19:28

bar: guard against invalid offset or width items

f4143e8

bar: remove use of forEach

125c537

bar: set base, offset and width default to null

f0a0c92

* Null defaults are used to denote optional properties. * Note that null values are ignored by `Lib.nestedProperty(/**/).set()`, so that the pattern `fullTrace.base === undefined` still works.

bar: add note to setGroupPositionsInOverlayMode

2ca7605

* Added note to explain the reason why setGroupPositionsInOverlayMode handles the case barnorm is defined.

bar: fix expansion of position axis

497121d

test: bar pinning tests

54a0967

* Commit bar tests before implementing the commit that interprets trace data as sizes. This should help ensure the implementation is thorough.

test: update bar base tests

6f58cd0

* Updated bar tests that set the base attribute, so that now trace data is interpreted as sizes.

bar: interpret trace data as sizes

bb93aba

test: update mock bar_attrs_group_norm

bad5c51

* Converted bar_attrs_group_norm into a horizontal bar plot.

test: update bar_attrs_group_norm baseline

6c6d680

* Updated baseline, because now this mock is a horizontal bar plot.

etpinard reviewed Oct 24, 2016

View reviewed changes

test/jasmine/tests/bar_test.js

});

});

describe('Bar.calc', function() {

Copy link

etpinard Oct 24, 2016

Choose a reason for hiding this comment

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

Very nice.

Thanks very much!

etpinard reviewed Oct 24, 2016

View reviewed changes

test/image/mocks/bar_attrs_group_norm.json

"type":"bar"

}, {

"base":[7,6,5,4],

"x":[1,2,3,4],

Copy link

etpinard Oct 24, 2016

Choose a reason for hiding this comment

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

very nice mock!

etpinard reviewed Oct 24, 2016

View reviewed changes

n-riesco mentioned this pull request Oct 24, 2016

Implement attributes base, offset and width in bar traces (issue 80) plotly/plotly.js#1075

Merged

n-riesco closed this Oct 27, 2016

n-riesco deleted the 20161017-issue-80-bar-base-offset-width branch October 27, 2016 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement attributes base, offset and width in bar traces (issue 80) #3

Implement attributes base, offset and width in bar traces (issue 80) #3

n-riesco commented Oct 21, 2016

etpinard commented Oct 21, 2016

etpinard Oct 21, 2016 •

edited

Loading

etpinard Oct 21, 2016

etpinard Oct 21, 2016

etpinard Oct 21, 2016

etpinard Oct 21, 2016

etpinard Oct 21, 2016 •

edited

Loading

n-riesco Oct 21, 2016

alexcjohnson Oct 21, 2016

alexcjohnson Oct 21, 2016

n-riesco Oct 21, 2016 •

edited

Loading

etpinard Oct 21, 2016

etpinard Oct 21, 2016

etpinard commented Oct 21, 2016 •

edited

Loading

alexcjohnson commented Oct 21, 2016

n-riesco commented Oct 24, 2016

etpinard Oct 24, 2016

alexcjohnson commented Oct 24, 2016

etpinard Oct 24, 2016

etpinard Oct 24, 2016

n-riesco Oct 24, 2016

etpinard Oct 24, 2016

n-riesco Oct 24, 2016

etpinard Oct 24, 2016

etpinard Oct 24, 2016

etpinard Oct 24, 2016

etpinard commented Oct 24, 2016

alexcjohnson commented Aug 31, 2017

Implement attributes base, offset and width in bar traces (issue 80) #3

Implement attributes base, offset and width in bar traces (issue 80) #3

Conversation

n-riesco commented Oct 21, 2016

etpinard commented Oct 21, 2016

etpinard Oct 21, 2016 • edited Loading

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 Oct 21, 2016 • edited Loading

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

n-riesco Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Oct 21, 2016 • edited Loading

alexcjohnson commented Oct 21, 2016

n-riesco commented Oct 24, 2016

Choose a reason for hiding this comment

alexcjohnson commented Oct 24, 2016

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 Oct 24, 2016

alexcjohnson commented Aug 31, 2017

etpinard Oct 21, 2016 •

edited

Loading

etpinard Oct 21, 2016 •

edited

Loading

n-riesco Oct 21, 2016 •

edited

Loading

etpinard commented Oct 21, 2016 •

edited

Loading