Cleanup specs for BubbleOverlay

[kum-deepak]

BubbleOverlay expects data corresponding to each of the points - removing empty bins interferes with this. See discussion in #1687.

For some reason - one of the circles are now different size. The new one seems correct. I am wondering was the old one incorrect?

[gordonwoodhull]

Thanks for pointing out the change in test result. This is why we have tests: not so much because we will immediately recognize what is the right result, but because when behavior changes we want to catch it and figure out why.

The test is for elastic radius. That is, when the chart is drawn, the extent of the input values from radiusValueAccessor is calculated and used as the domain for the bubble radius scale r.

Now I vaguely recall that when I lifted elasticRadius to the bubble mixin in #661, I had to add removeEmptyBins to the test, or it wouldn't have the right effect. As you can see, the previous test observed the smaller bubble snapping to 10, the minimum radius. But without removeEmptyBins, the still-present zeros are snapped to 10, and the smaller visible bubble is around 45.

So to go to the root of the problem, #661 was not as simple as it looked. In order to implement elastic radius correctly on the bubble overlay chart, since it does not support removeEmptyBins, the zeros need to be excluded when calculating the elastic radius.

Interestingly, the bubble mixin already has a special case for zeros when it sets the SVG radius:

dc.js/src/base/bubble-mixin.js

Lines 114 to 121 in 64716df

	bubbleR (d) {
	const value = this.radiusValueAccessor()(d);
	let r = this.r()(value);
	if (isNaN(r) || value <= 0) {
	r = 0;
	}
	return r;
	}

It passes the value into the radius scale, and then uses 0 if the result is NaN or the value is 0.

To fix this, rMin() needs to have a similar special case, removing the zeros before computing the minimum:

dc.js/src/base/bubble-mixin.js

Lines 106 to 108 in 64716df

	rMin () {
	return min(this.data(), e => this.radiusValueAccessor()(e));
	}

Something like

return min(this.data().map(this.radiusValueAccessor()).filter(value => value > 0))

[gordonwoodhull]

Tested the change and reverted the change to the test!

Thanks @kum-deepak!

[gordonwoodhull]

Rats, this broke the iris tests of the bubble chart.

I guess either choice might be valid, so I'm adding an option called excludeElasticZero and defaulting it true. As noted above, I think it is more consistent to exclude zeros when calculating elastic, because any zeros in the data are special cased to cause the radius to be zero.

However, there might be reasons why people like the old behavior, so a flag is the safest thing to do.

By analogy, consider #667, where the current behavior is to always include zero in elasticY, but the issue asks to optionally not include zero, occasionally a valid choice.

[kum-deepak]

Wow! So many things going on while I thought it was a simple one 😄

[gordonwoodhull]

Yeah, it seems to be the rule for charting libraries: features will interact in complex ways. 😔