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

n-riesco · 2016-10-24T21:08:31Z

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

This PR's review can be found here.

* 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.

* 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.

* Moved shared code between `setOffsetAndWidth` and `setOffsetAndWidthInGroupMode` to handle offset and width attributes into function `applyAttributes`.

* Test the use of the bar attributes: base, offset and width.

* Added mock images of bar plots using the attributes base, offset and width.

etpinard · 2016-10-24T21:18:11Z

@n-riesco great. I'll give this PR one last test drive tomorrow morning (UTC-4)

* This change is needed so that the attribute `base` in bar traces can accept `number` and `Date` values.

n-riesco · 2016-10-24T22:51:47Z

@etpinard

plotschema_test.js fails because of attribute base. base can take a data value (valType: 'any') or a data array (valType: 'data_array').

I guess the easiest workaround would be to use valType: 'any' and remove arrayOk.

etpinard · 2016-10-25T00:26:31Z

Can base be anything other than a number or an array of numbers?

If so, then it should be valType: 'number' with arrayOk: true.

n-riesco · 2016-10-25T12:24:15Z

In principle, base could be any value taken by the corresponding axis. I can't think of any meaningful bar plot, where the size axis isn't a number, though.

@alexcjohnson How do you think? Did you have anything in mind in #80 that would require base to be any?

alexcjohnson · 2016-10-25T12:44:05Z

In general if there's no concept of zero on an axis then bar charts don't make sense. Though interestingly I guess with base we could relax that requirement and say distance must be meaningful (and the size data would be expressed as such a distance) but you don't need a zero. Date axes match that description, and the corresponding chart is a Gantt chart (cc @jackparmer ) so if we want to support that I guess base would need to be 'any' but size would still be a number (milliseconds)

n-riesco · 2016-10-25T13:37:11Z

@etpinard @alexcjohnson Here's a crazy idea. At the moment, plotly classifies as valType: 'number' anything that isNumeric says it's a number. If isNumeric would consider Date values as numbers, then base declared as {valType: 'number', arrayOk: true} would also accept date values. Do you think this would work?

etpinard · 2016-10-25T13:42:59Z

If isNumeric would consider Date values as numbers, then base declared as {valType: 'number', arrayOk: true} would also accept date values. Do you think this would work?

Hmm. I afraid that patching isNumeric will lead to some issues downstream. I'm thinking we should add a new valType called coordinate for attributes of the likes - in a future PR.

So, let's go with { valType: 'any', arrayOk: true } in this PR.

Thanks for the heads-up @n-riesco !

n-riesco · 2016-10-25T13:53:23Z

@etpinard Do you want me to add arrayOk to

plotly.js/src/lib/coerce.js

Line 231 in ec0b0a4

otherOpts: ['dflt', 'values'],

so that plotschema_test.js doesn't fail?

alexcjohnson · 2016-10-25T13:56:07Z

coordinate is an interesting idea... though you could say categories are coordinates too so it doesn't specify much. but yeah, dates and numbers are generally not interchangeable so we need to manage them separately.

I wonder if there would be value in valType unions, so you could specify number or date string, for example.

etpinard · 2016-10-25T13:57:53Z

Do you want me to add arrayOk to

plotly.js/src/lib/coerce.js

Line 231 in ec0b0a4

otherOpts: ['dflt', 'values'],

so that plotschema_test.js doesn't fail?

yes please.

etpinard · 2016-10-25T15:20:34Z

Oh @n-riesco I forgot about something ... Plotly.restyle !

We (unfortunately - see #648) need to manually list which attributes require going through the calc / set-position on updates. That list is getting really ugly, but luckily the patch is easy to make:

should be all you need.

While doing so, maybe you can add one restyle test in bar_test.js. Requiring one test mock e.g require('@mocks/bar_attrs_relative'), together with a few Plotly.restyle calls and a few expect(gd.calcdata) should be all you need.

Sorry about the late notice.

* Fixed the bar tests that make use of the attribute offset. The values of the bar center in these tests were incorrect.

* Refactored `setWidthAndOffset` and `setWidthAndOffsetInGroupMode` so that the bar center is computed after applying the trace attributes offset and width.

n-riesco · 2016-10-25T19:53:45Z

@etpinard While writing therestyle test, I noticed a bug in the calculation of the bar centers and the corresponding tests (bar centers have to be computed after applying offset and width). This is now fixed.

etpinard · 2016-10-25T20:48:28Z

@n-riesco you win the PR of week 🏆

Thanks very much. This should make a bunch of people 😄

n-riesco added 29 commits October 24, 2016 21:49

bar: use descriptive variable names

abab672

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

bar: convert closure into function

c8b33f0

* 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

2a72e97

* Moved code to group bars from setGroupPositions to setPositions.

bar: rename barpositions to setOffsetAndWidth

3dd6ed0

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

bar: refactor code into function setBaseAndSize

f63a7b7

bar: Move code for overlay mode into a function

c150b3d

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

bar: use Sieve to stack bars in setBaseAndSize

57deb7b

bar: add functions stackBars and normalizeBars

8dfa0bd

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

bar: fix image test failure in hist_grouped

c61e642

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

bar: do not normalise bars with no size

b25467b

bar: Add attributes base, offset and width

54ac1f1

bar: add function sieveBars

23bdc82

* 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

19e114e

bar: ensure size axis includes bar bases

f2a365e

bar: do not stack traces that set bar base

83fdb44

bar: implement attributes base, offset and width

8d75c3a

bar: guard against invalid base items

2e8c7a1

bar: guard against invalid offset or width items

dbbf8e7

bar: remove use of forEach

655b3b9

bar: set base, offset and width default to null

1eaa130

* 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

83168bd

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

bar: fix expansion of position axis

5d4c84a

bar: interpret trace data as sizes

b4732da

bar: add function applyAttributes

7440346

* Moved shared code between `setOffsetAndWidth` and `setOffsetAndWidthInGroupMode` to handle offset and width attributes into function `applyAttributes`.

bar: document purpose of updatePositionAxis

152fefe

test: update bar tests

a12872d

* Test the use of the bar attributes: base, offset and width.

test: add more mock images of bar plots

c3638a6

* Added mock images of bar plots using the attributes base, offset and width.

etpinard added the feature something new label Oct 24, 2016

etpinard added the status: reviewable label Oct 24, 2016

etpinard added this to the v1.19.0 milestone Oct 24, 2016

coerce: allow arrayOk and valType: 'any'

db7105d

* This change is needed so that the attribute `base` in bar traces can accept `number` and `Date` values.

n-riesco added 4 commits October 25, 2016 20:30

test: fix bar tests

339f5c9

* Fixed the bar tests that make use of the attribute offset. The values of the bar center in these tests were incorrect.

bar: set bar center after applying attributes

cec4ba0

* Refactored `setWidthAndOffset` and `setWidthAndOffsetInGroupMode` so that the bar center is computed after applying the trace attributes offset and width.

bar: fix restyle of bar base, width and offset

e4f4306

test: restyle of bar base, width and offset

cc69e8f

etpinard merged commit 3cb7e7e into plotly:master Oct 25, 2016

This was referenced Oct 25, 2016

Make base of bars configurable #475

Closed

explicit bar positioning and width #80

Closed

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

This was referenced Nov 1, 2016

Hover text appears at wrong position in bar charts #1105

Closed

Fix hover bar delta #1107

Merged

plotly deleted a comment from AdnanBoota Aug 31, 2017

chriddyp mentioned this pull request Nov 8, 2017

Bullet Chart FF plotly/plotly.py#872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

n-riesco commented Oct 24, 2016

etpinard commented Oct 24, 2016

n-riesco commented Oct 24, 2016

etpinard commented Oct 25, 2016

n-riesco commented Oct 25, 2016

alexcjohnson commented Oct 25, 2016

n-riesco commented Oct 25, 2016

etpinard commented Oct 25, 2016

n-riesco commented Oct 25, 2016

alexcjohnson commented Oct 25, 2016

etpinard commented Oct 25, 2016

etpinard commented Oct 25, 2016

n-riesco commented Oct 25, 2016

etpinard commented Oct 25, 2016

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

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

Conversation

n-riesco commented Oct 24, 2016

etpinard commented Oct 24, 2016

n-riesco commented Oct 24, 2016

etpinard commented Oct 25, 2016

n-riesco commented Oct 25, 2016

alexcjohnson commented Oct 25, 2016

n-riesco commented Oct 25, 2016

etpinard commented Oct 25, 2016

n-riesco commented Oct 25, 2016

alexcjohnson commented Oct 25, 2016

etpinard commented Oct 25, 2016

etpinard commented Oct 25, 2016

n-riesco commented Oct 25, 2016

etpinard commented Oct 25, 2016