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

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

Author: alexcjohnson

src/traces/histogram/calc.js

@@ -233,11 +233,6 @@ function calcAllAutoBins(gd, trace, pa, maindata) {
         // But manually binned traces won't be adjusted, even if the auto values
         // are inconsistent with the manual ones (or the manual ones are inconsistent
         // 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.
-        // Need to look for cases like this and see if the results are acceptable
-        // or we need to think harder about it.
         minSize = getMinSize(minSize, binspec.size);
         minStart = Math.min(minStart, pa.r2c(binspec.start, 0, calendar));
         maxEnd = Math.max(maxEnd, pa.r2c(binspec.end, 0, calendar));
@@ -283,10 +278,10 @@ function calcAllAutoBins(gd, trace, pa, maindata) {
 }
 
 /*
- * 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...
+ * Return an array of traces that are all stacked or grouped together
+ * Only considers histograms. In principle we could include them in a
+ * similar way to how we do manually binned histograms, though this
+ * would have tons of edge cases and value judgments to make.
  */
 function getConnectedHistograms(gd, trace) {
     if(gd._fullLayout.barmode === 'overlay') return [trace];

test/jasmine/tests/histogram_test.js

@@ -282,14 +282,49 @@ describe('Test histogram', function() {
             var trace1 = {x: [1, 2, 3, 4]};
             var trace2 = {x: [5, 5.5, 6, 6.5]};
 
-            expect(calcPositions(trace1)).toEqual([0.5, 2.5, 4.5]);
+            expect(calcPositions(trace1)).toBeCloseToArray([0.5, 2.5, 4.5], 5);
 
-            expect(calcPositions(trace2)).toEqual[5, 6, 7];
+            expect(calcPositions(trace2)).toBeCloseToArray([5.5, 6.5], 5);
 
             expect(calcPositions(trace1, [trace2])).toEqual([1, 2, 3, 4]);
+            // huh, turns out even this one is an example of "unexpected bin positions"
+            // (see another example below) - in this case it's because trace1 gets
+            // autoshifted to keep integers off the bin edges, whereas trace2 doesn't
+            // because there are as many integers as half-integers.
+            // In this case though, it's unexpected but arguably better than the
+            // "expected" result.
             expect(calcPositions(trace2, [trace1])).toEqual([5, 6, 7]);
         });
 
+        it('can sometimes give unexpected bin positions', function() {
+            // documenting an edge case that might not be desirable but for now
+            // we've decided to ignore: a larger bin sets the bin start, but then it
+            // doesn't quite make sense with the smaller bin we end up with
+            // we *could* fix this by ensuring that the bin start is based on the
+            // same bin spec that gave the minimum bin size, but incremented down to
+            // include the minimum start... but that would have awkward edge cases
+            // involving month bins so for now we're ignoring it.
+
+            // all integers, so all autobins should get shifted to start 0.5 lower
+            // than they otherwise would.
+            var trace1 = {x: [1, 2, 3, 4]};
+            var trace2 = {x: [-2, 1, 4, 7]};
+
+            // as above... size: 2
+            expect(calcPositions(trace1)).toBeCloseToArray([0.5, 2.5, 4.5], 5);
+
+            // {size: 5, start: -5.5}: -5..-1, 0..4, 5..9
+            expect(calcPositions(trace2)).toEqual([-3, 2, 7]);
+
+            // unexpected behavior when we put these together,
+            // because 2 and 5 are mutually prime. Normally you could never get
+            // groupings 1&2, 3&4... you'd always get 0&1, 2&3...
+            expect(calcPositions(trace1, [trace2])).toBeCloseToArray([1.5, 3.5], 5);
+            expect(calcPositions(trace2, [trace1])).toBeCloseToArray([
+                -2.5, -0.5, 1.5, 3.5, 5.5, 7.5
+            ], 5);
+        });
+
         it('harmonizes autobins with smaller manual bins', function() {
             var trace1 = {x: [1, 2, 3, 4]};
             var trace2 = {x: [5, 6, 7, 8], xbins: {start: 4.3, end: 7.1, size: 0.4}};
@@ -315,7 +350,7 @@ describe('Test histogram', function() {
             var trace4 = {x: [1, 1.2, 1.4, 1.6], yaxis: 'y2'};
 
             expect(calcPositions(trace1, [trace2, trace3, trace4])).toEqual([1, 2, 3, 4]);
-            expect(calcPositions(trace3)).toBeCloseToArray([0.9, 1.1, 1.3]);
+            expect(calcPositions(trace3)).toBeCloseToArray([0.9, 1.1, 1.3], 5);
         });
 
         describe('cumulative distribution functions', function() {