Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scatterplot is setting its own key & value accessors #702

Open
gordonwoodhull opened this issue Sep 12, 2014 · 4 comments
Open

scatterplot is setting its own key & value accessors #702

gordonwoodhull opened this issue Sep 12, 2014 · 4 comments
Assignees
Labels
Milestone

Comments

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Sep 12, 2014

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

@gordonwoodhull gordonwoodhull self-assigned this Sep 12, 2014
@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Sep 12, 2014
gordonwoodhull added a commit that referenced this issue Sep 12, 2014
This is not a complete fix for #702, but at least it allows reducing multiple
values at once.  (It is still very weird that the valueAccessor is set to
look at part of the key.)
@gordonwoodhull
Copy link
Contributor Author

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.

@gordonwoodhull
Copy link
Contributor Author

For reference, the suspicious accessor overrides are here:
https://github.com/dc-js/dc.js/blob/master/src/scatter-plot.js#L39-41

@gordonwoodhull gordonwoodhull modified the milestones: v2.1, v2.0 Sep 13, 2014
@gordonwoodhull gordonwoodhull changed the title rethink the way scatterplot is using key & value accessors scatterplot is setting its own key & value accessors Dec 29, 2014
@gordonwoodhull
Copy link
Contributor Author

For reference, this was introduced in 6adeeed to help with scatterplot brushing. I'm sure there is a better way to do this.

@stevemandl
Copy link

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.

gordonwoodhull added a commit that referenced this issue Sep 16, 2015
cherry-picking @vprus's fix from master (#702/#704)
tests are still massively broken and won't be fixed!
gordonwoodhull added a commit that referenced this issue Dec 23, 2016
the scatter-series one i understand - we have to prevent the composite
chart from overriding the title function, since we need the special
title function that overrides the grotesque #702 scatter override of
key+value accessors

but i can't get why we don't need that for the multi-scatter
example. they're both composite charts, but this one works with the
shared composite title function because somehow or another, the key
accessor that title function sees is *not* the scatter override one.

it has something to do with the title function capturing key+value
accessors from its own closure, but i'm confused as heck and must be
tired as well.

ref #1200
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants