dc-js
scatterplot is setting its own key & value accessors
Right now, the scatterplot sets its own value accessor that looks at part of the key, presumably so that the coordinateGridMixin can use the value accessor to get Y values
And then in a couple of places it accesses d.value directly so that it knows whether a bubble exists or not.
Both of these things are somewhat wrong.
First, for a scatterplot, Y is really part of the key, not a value. If there are necessary assumptions in the coordinateGridMixin that value is Y, then the scatterplot is not really a coordinateGrid.
Second, we should never access d.value directly but should use accessors, so that people can reduce to whatever they want, not just single values. This part breaks reducing colors or sizes, for example. We should always support multivalue reductions, everywhere.
https://groups.google.com/forum/#!topic/dc-js-user-group/55nJcU0qDfg
Fixed the d.value part in the commit above.
However, it is still weird to have the chart set its own key and value accessors. For one thing, I'm pretty sure this means that it's not possible for users to set their own key accessor in the usual way, or they will have to include the same strange logic in their own accessor. The chart's constructor will run before the user's keyAccessor call, so the user will override it.
For reference, the suspicious accessor overrides are here: https://github.com/dc-js/dc.js/blob/master/src/scatter-plot.js#L39-41
For reference, this was introduced in 6adeeed35ac45117c67baf208b10bddf90409b69 to help with scatterplot brushing. I'm sure there is a better way to do this.
I have been looking into a better way to do this also.
Perhaps coordinateGridMixin should be split up:
- Create two flavors of mixins, the current coordinateGridMixin, and the other (xyMixin?, keyBasedCoordinateGridMixin?) the same in many respects, except:
- omit things like ordinalXDomain.
- Add x and y accessors that operate on the key (the dimension used would have to encode the coordinates in the key somehow)
- Organize the mixins in whatever way facilitates code reuse and maintainability. (perhaps the common code could go in xyMixin, then both coordinateGridMixin and keyBasedXYMixin could include the xyMixin)
- Implement 2d brushing to use a function that uses these accessors.
The intention is to change scatterPlot to include this new keyBasedXYMixin instead of coordinateGridMixin.
RangedTwoDimensionalFilter could be extended to accept x,y accessors and incorporate them into the resulting filter. Then the isFiltered() in the returned filter could use these accessors insead of the fixed accessors. Since they operate only on the key, the same accessors would work in this context too. RangedTwoDimensionalFilter would only work on dimensions that encode the xy coordinates in the key, such as those used in key based xy charts.
This proposal would not allow filtering based on the key/value accessors: the 2d filter applied to a key based xy chart would operate on the dimension's key, not on the reduced value. The coordinates of points on a chart using this new mixin would never move when other filters were applied, such as bubbles in a bubble chart currently can do.
For charts based on coordinateGridMixin, 'simulated' 2d brushing could be implemented, where the simulated brush is only rendered when brushing, and simply leaves behind a filter function that includes the set of dimension keys rendered within the brush region. This result would be exactly as if the user had selected all of the elements within the selected polygon individually, but would essentially be a UI convenience function. In case it is not apparent, what I'm describing here is NOT using RangedTwoDimensionalFilter. After applying a simulated brush, if filters to another dimension were applied causing the coordinates of filtered elements to change (which could happen if the key or value accessors were dependent on reduced values), the shape of the 'simulated' brush may change, but the same dimension keys would remain filtered. Issues such as rendering the brush in the zoomHandler would go away, since the simulated brush is only be rendered when brushing.
The suggestion in the last paragraph could be added to the existing coordinateGrid without the changes proposed in the previous paragraphs.
