Match hist bins #1944

alexcjohnson · 2017-08-10T07:05:37Z

All autobinned histograms on a given subplot, when barmode is not 'overlay', get the same bin size and alignment
That size is the minimum any of them were auto-assigned
If there are any manually binned histograms in the group, match the autobinned ones to the first manual one:
- More than one manual histogram, if they're inconsistent there's no way to fully satisfy the constraints so don't even bother looking at it, just use the first one.
- In case the manual bins are larger than the autobins but not as large as the next larger autobin step, expand (and shift) the autobins to match exactly
- Otherwise shrink the autobins to the next largest integer fraction of the manual bin size. Align the bin edges so that there is an autobin center at the center of each manual bin. In this case the manual bins will still be represented incorrectly when they're drawn (with the narrower width of the autobins) but there's nothing we can do about that (stacked/grouped bars must have matching widths) at least the autobins will be drawn with the correct width.

alexcjohnson · 2017-08-10T07:14:46Z

src/traces/histogram/calc.js

+        // with each other).
+        //
+        // TODO: there's probably a weird case here where a larger bin pushes the
+        // start/end out, then it gets shrunk and doesn't make sense with the smaller bin.


In a small set of cases, by fixing the major problem (bin sizes getting drawn wrong) we introduce a smaller problem (bin edges get shifted so an undesirably large fraction of data is ambiguously right at bin edges). This is possible because we set minSize and minStart independently so they can come from different traces. The vast majority of the time though the bin edges are still positioned nicely at the end of this, and the fix would need to involve keeping minStart based on the same trace that gave minSize while expanding it to encompass all of these traces, which wouldn't be too hard for numeric bins but gets tricky for 'M<#>' bins. To me then this isn't worth fixing.

I don't mind not fixing this either. It would be nice to add a test case to document this behavior though.

tested in ec12eab

alexcjohnson · 2017-08-10T07:21:19Z

src/traces/histogram/calc.js

+ * return an array of traces that are all stacked or grouped together
+ * TODO: only considers histograms. Should we also harmonize with bars?
+ * in principle people can mix and match these, but bars always
+ * specify their positions explicitly...


We could try to treat bars the same as we treat manually-binned histograms in adjusting the autobins. That has more edge cases to consider though, nonuniform bar spacing being perhaps the hardest to manage, so I'm inclined not to worry about this one either. If someone is grouping/stacking bars with histograms I'd like to think it would be clear to them that they need to set the histogram binning to match the bar positions...

I'm inclined not to worry about this one either

No worries for me either.

etpinard · 2017-08-14T18:31:45Z

src/traces/histogram/calc.js

+    // the raw data was prepared in calcAllAutoBins (during the first trace in
+    // this group) and stashed. Pull it out and drop the stash
+    var pos0 = trace._pos0;
+    delete trace._pos0;


Maybe calcAllAutoBins should return a tuple e.g. [binspec, pos0]?

perhaps... we'd still need to stash _pos0 because it could be used in a totally separate call to calc, but at least that could all be dealt with inside calcAllAutoBins, so yeah that would be easier to understand. 👍 (unless we end up moving to setPositions...)

reorg so _pos0 is only in calcAllAutoBins in 89093ad

etpinard · 2017-08-14T18:39:52Z

src/traces/histogram/calc.js

+
+    // all but the first trace in this group has already been marked finished
+    // clear this flag, so next time we run calc we will run autobin again
+    if(trace._autoBinFinished) {


Maybe we should move auto-bins computations to histograms/set_positions (which loops over all traces of a given trace type) to avoid having to use _ flags like this one?

... maybe histogram/calc.js should only sanitize pos and size values and call arraysToCalcdata, and leave the rest to set_positions.js?

maybe histogram/calc.js should only sanitize pos and size values and call arraysToCalcdata, and leave the rest to set_positions.js?

Pro: that would avoid coupling between traces in a part of the code (calc) that's supposed to be operating on one trace at a time... and set_positions is only called after calc (as opposed to every replot), which is when it's needed.

Con: we'd need to be saving these sanitized pos0 during calc, rather than making an actual calcdata[i] array - or perhaps stashing it in calcdata[i][0] as we used to do with trace-wide stuff (and still do here and there) but then setPositions would need to fill in the rest of calcdata[i] once it determined the bin spec.

So I feel like in the end it would be more confusing that way. Thoughts?

Incidentally, it looks like we're probably calling setPositions twice unnecessarily if we have both histogram and bar traces on the same plot. I'll investigate...

So I feel like in the end it would be more confusing that way. Thoughts?

Good point. I don't have a strong opinion. This whole calc vs set_positions debate was just something that came to mind, not anything blocking that's for sure.

OK cool - I'm going to leave it then, but this discussion will be useful to keep in mind for the future, I suspect at some point we will want to reimagine the whole pipeline to be a bit more flexible - particularly in regards to minimizing redraw work.

Incidentally, it looks like we're probably calling setPositions twice unnecessarily if we have both histogram and bar traces on the same plot. I'll investigate...

Yes we were. Fixed in e81fcb6 - I couldn't think of an easy way to test that we're not doing extra work, but we do have a test that this didn't break anything, in bar_and_histogram.json. I also had it avoid even doing the setPositions loop when there are no setPositions functions to call.

etpinard · 2017-08-14T18:42:00Z

test/jasmine/tests/histogram_test.js

+            jasmine.addMatchers(customMatchers);
+        });
+
+        function _calc(opts, extraTraces) {


This PR probably deserves an image test too.

modified test images in 3f173a4 - previously trace1 would have gotten bin size 2, so bin centers at 1 & 2 for trace0, 0.5, 2.5, and 4.5 for trace1 and displayed bar widths of 0.5. Now they all get bin size = displayed bar width = 1

also fixed a typo in the test that means... we were actually already sort of supposed to be testing the TODO about start/size interactions

etpinard · 2017-08-15T00:54:53Z

src/traces/histogram/calc.js


-    return trace[binAttr];
+    return [trace[binAttr], pos0];


Awesome. Thanks!

etpinard · 2017-08-15T02:11:12Z

💃

though we should probably add a few examples on https://github.com/plotly/documentation to help out users at some point.

FYI @alexcjohnson, the next oldest bug is #109 but maybe I should take that one 😄

alexcjohnson · 2017-08-15T02:27:41Z

Haha you noticed my pattern 🥂 Some of them are getting a liiittle too old.

jurasource · 2017-09-20T09:39:25Z

This seems to generate errors when I have multiple traces. Only happens when I toggle one on/off a few times.
I managed to narrow down the bug to release 1.30.0.
No example I'm afraid, jsfiddle etc are blocked by my firewall.

plotly-latest.js:168259 Uncaught TypeError: Cannot read property 'length' of undefined at Object.calc (plotly-latest.js:168259) at Object.829.plots.doCalcdata (plotly-latest.js:152035) at 753.Plotly.plot (plotly-latest.js:130164) at Object.725.lib.syncOrAsync (plotly-latest.js:126168) at Object.restyle (plotly-latest.js:131297) at handleClick (plotly-latest.js:116491) at plotly-latest.js:116410

etpinard · 2017-09-20T13:38:44Z

@jurasource that error looks similar to #2020 - can you confirm?

jurasource · 2017-09-20T13:50:18Z

@etpinard yes, it's the same error

alexcjohnson added 2 commits August 10, 2017 00:15

histogram autobins match each other when possible

e5a535c

extra test for histograms on other axes

11821c2

alexcjohnson commented Aug 10, 2017

View reviewed changes

alexcjohnson added status: reviewable bug something broken labels Aug 10, 2017

etpinard reviewed Aug 14, 2017

View reviewed changes

alexcjohnson added 5 commits August 14, 2017 18:35

clear - and test - TODOs we're not going to address now

ec12eab

also fixed a typo in the test that means... we were actually already sort of supposed to be testing the TODO about start/size interactions

test new histogram logic in images

3f173a4

handle _pos0 totally inside calcAllAutoBins

89093ad

camelCase histogram.calc

2071acb

cut out duplicate setPositions calls (and loops)

e81fcb6

etpinard reviewed Aug 15, 2017

View reviewed changes

src/traces/histogram/calc.js Outdated

return trace[binAttr];

return [trace[binAttr], pos0];

Copy link

Contributor

etpinard Aug 15, 2017

Choose a reason for hiding this comment

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

Awesome. Thanks!

etpinard added this to the v1.30.0 milestone Aug 15, 2017

pushUnique setPositions

241d9a8

alexcjohnson merged commit fd54bee into master Aug 15, 2017

alexcjohnson deleted the match-hist-bins branch August 15, 2017 02:28

This was referenced Aug 15, 2017

Stacked Histogram not rendering correctly when sample sizes are different #1864

Closed

histogram autobins plotly/documentation#799

Open

alexcjohnson mentioned this pull request Sep 7, 2017

"xbins" on Histogram not working Correctly #1978

Closed

alexcjohnson mentioned this pull request Mar 7, 2018

Single-bar histograms don't respect manual bins specs when data lies in final bin #1229

Closed

alexcjohnson mentioned this pull request Sep 12, 2018

Reimagining histogram autobin #3001

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match hist bins #1944

Match hist bins #1944

alexcjohnson commented Aug 10, 2017

alexcjohnson Aug 10, 2017

etpinard Aug 14, 2017

alexcjohnson Aug 14, 2017

alexcjohnson Aug 10, 2017

etpinard Aug 14, 2017

etpinard Aug 14, 2017

alexcjohnson Aug 14, 2017

alexcjohnson Aug 14, 2017

etpinard Aug 14, 2017

etpinard Aug 14, 2017 •

edited

Loading

alexcjohnson Aug 14, 2017

etpinard Aug 14, 2017

alexcjohnson Aug 14, 2017

alexcjohnson Aug 14, 2017

etpinard Aug 14, 2017

alexcjohnson Aug 14, 2017

etpinard Aug 15, 2017

etpinard commented Aug 15, 2017 •

edited

Loading

alexcjohnson commented Aug 15, 2017

jurasource commented Sep 20, 2017

etpinard commented Sep 20, 2017

jurasource commented Sep 20, 2017

Match hist bins #1944

Match hist bins #1944

Conversation

alexcjohnson commented Aug 10, 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

etpinard Aug 14, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Aug 15, 2017 • edited Loading

alexcjohnson commented Aug 15, 2017

jurasource commented Sep 20, 2017

etpinard commented Sep 20, 2017

jurasource commented Sep 20, 2017

etpinard Aug 14, 2017 •

edited

Loading

etpinard commented Aug 15, 2017 •

edited

Loading