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

Bullet Chart #101

Closed
wants to merge 2 commits into
base: master
from

Conversation

2 participants
@jasondavies
Collaborator

jasondavies commented Apr 6, 2011

This is based on the Protovis version. This initial commit is just to see if I'm on the right track, there are still a few things to add e.g. vertical orientation before it should be pulled.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 7, 2011

Some quick feedback, since I don't have time this morning to give it justice. This looks great!

I would call this a "chart" rather than a "layout". The layouts in D3 (unlike Protovis) are representation-independent. That is, they create helper data structures that can then be used to control the layout of graphical elements. For example, the force layout creates node objects with x and y fields; these objects are then used to position svg:circle elements or whatever representation is desired to display them. Similarly the other layouts create (or modify, in the case of stack) data structures rather than directly rendering anything.

So, I propose we create a new chart module for D3, and put this in d3.chart.js.

I like the API using call and embedding the chart widget inside an svg:g.

I think you can make the chart implementation more flexible by using functions for the data, rather than passing in constants. In fact, what I would do as the simplest implementation would be to depend on data that is bound to the containing svg:g element. A data structure like this would work well:

{
  "ranges": [2.1e6, 2.85e6, 4e6],
  "measures": [2.3e6, 3.01e6],
  "markers": [1.15e6, 3.0e6]
}

Then, inside bullet.js, you'd initialize your private variables a bit differently:

ranges = function(d) { return d.ranges; },
measures = function(d) { return d.measures; },
markers = function(d) { return d.markers; },

But, note that your code later on, when you call the data operator, stays the same!

OK, I'm glossing over a bit. There's actually a tricky part where you want to evaluate the ranges/measures/markers ahead of time, so you can compute the rangeColor/measureColor/scale etc. But ideally we figure out a way to do this so that the bullet chart works automatically for small multiples. What I'd like to see is if I call a bullet chart on a selection with multiple svg:g elements (each with their own data structure already bound, as above), the chart implementation correctly generates small multiples. But note that you'll want the scale to be consistent across the multiples, so some gymnastics is needed to pull out the data from the this selection and do initialization!

Let me know if this makes sense / doesn't make sense. If I get some time later, I can help hack!

Cheers,
Mike

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 7, 2011

Wonderful, thanks! I was wondering how to make it generate small multiples - your idea of relying on bound data sounds great.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 7, 2011

Okay, I've got bullet multiples working now. I'm a bit unsure about when to refresh the scales, should this be exposed as .refresh() to be called if/when the data changes? Should this also apply some kind of transition, and how should the transition be customisable?

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 7, 2011

Ah yes, that's the other thing. Transitions should be totally doable by re-selecting existing elements within the svg:g, rather than assuming it's empty. Can't look now but will look again later! Ciao.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 7, 2011

Thanks, I've added transitions by default now, but ideally they wouldn't apply the first time round. Maybe the transition delay could be an optional argument, so you could do d3.select('g.bullet').call(bullet, 100) for a 100ms transition duration. Perhaps the default could be 0 (internally it would probably just avoid .transition() until passing 0 results in immediate application).

I did try variations of d3.select('g.bullet').transition().call(bullet) but this doesn't really work for various reasons, for example you can't really append() new nodes to a transition.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 7, 2011

Thanks. I've actually changed it to set the fill attribute directly instead now as that's what I've done elsewhere.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 7, 2011

Yeah, if you want to avoid transitions on enter you can split the code into enter + update. For example:

var range = chart.selectAll('rect.range')
    .data(ranges);

range.enter().append('svg:rect')
    .attr('class', 'range')
    .attr('width', scale)
    .attr('height', height)
    .style('fill', function(d, i) { return rangeColor(i); });

range.transition()
    .attr('width', scale)
    .attr('height', height)
    .style('fill', function(d, i) { return rangeColor(i); });

Now there's a bit of code duplication, which you could avoid by a call to a function that sets width + height + fill. And actually in this case the range colors can't change, so you don't technically need to update them (though it might be a good idea to allow configurable colors in the future).

The other issue is that the number of ranges could change, so you might want to exit().remove()? You could define special transitions for enter and exit as well, if you wanted to fancy!

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 7, 2011

Wow. Just looked at bullet-multiples.html! AWESOME!

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 7, 2011

Hehe :-) Still need to add the other orientations, and clean up the height computation. I think I should probably add title and subtitle support too like in the Protovis version.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 7, 2011

Bedtime for me, here are some random thoughts in case I forget:

  • Still wondering about making transitions more flexible. One way would be to expose the internal transition function as a property, so that other easings can be specified. I wonder if people would really need it though. Less is more!
  • Orientations: might be simpler to use a few SVG transforms instead of doing lots of conditionals for each x, y, width, height, etc. attribute.
  • Also need to add other marker types. Another property, specifying a symbol type to pass to d3.svg.symbol()?

Feel free to hack on it if you want! I'll probably look at it again tomorrow.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 8, 2011

OK, I took a pass at it. Mainly, I cleaned up the transitions so there's object constancy for ticks, and we deal correctly with changing scales (entering objects should be initialized with the old x-scale, then transition to the new scale as they fade in).

I restructured the code a bit. That wasn't really intentional---sorry if that makes the merge difficult. I'm open to ideas on how to reduce the code duplication between enter + update + exit, but I often find that there is always slightly different operators needed for these three phases, so refactoring into a reusable function doesn't help much.

Anyway, take a look. The transitions are slick!

1b1a948

If you want, pull from my 'bullet' branch, and then keep hacking. Then we can pull this into the master branch when you're ready.

One related question: should we track this under the same master version number? In which case any non-backwards-compatible change to the chart API requires bumping the major version number? Or should we have a separate version number for separate libraries? I have a feeling the latter fragmentation will be confusing. Probably we should be conservative about backwards-incompatible changes and use a single version number.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 8, 2011

I think as it's in the same git repository keeping the same master version number makes sense for anyone who uses github to host particular versions (as you do for bl.ocks.org examples). Unless you're thinking of using a combined version tag of some kind? I think separate versions for all non-core libraries could certainly get a bit confusing. Also, you may then run into dependency issues.

Maybe it's something to consider if/when the chart library gets a lot bigger: it could then be put into a separate repository with its own version number. This would mean core library development wouldn't be slowed down by having to ensure all the charts are compatible with ever backwards-incompatible change.

So yes, a single version number seems best, at least for now.

chart.filter(function(d) { return d === orient; })
.selectAll('g.bullet').call(bulletChart);
});
};

This comment has been minimized.

@mbostock

mbostock Apr 8, 2011

Member

Oh, I see. You're creating separate layouts with different orientations. I was confused for a bit. So, ya, an easy way to create a selection from the current node is d3.select(this):

var bullet = function(vis) {
  vis.each(function(orient) {
    d3.select(this).selectAll("g.bullet")
        .call(d3.chart.bullet()
        .orient(orient)
        .duration(1000));
  });
};

Alternatively, you could just change the call on L37 to vis.each(bullet), in which case:

var bullet = function(orient) {
  d3.select(this).selectAll("g.bullet")
      .call(d3.chart.bullet()
      .orient(orient)
      .duration(1000));
};

This comment has been minimized.

@jasondavies

jasondavies Apr 8, 2011

Collaborator

Ah thanks, I prefer the latter, I'll add it now...

var orient = "left",
duration = 0,
title = d3_chart_title,
subtitle = d3_chart_subtitle,

This comment has been minimized.

@mbostock

mbostock Apr 8, 2011

Member

I use the prefix d3_{module}_{class} to avoid naming collisions. It's a bit verbose, but it gets minimized anyway. :)

This comment has been minimized.

@jasondavies

jasondavies Apr 8, 2011

Collaborator

Hehe, hadn't quite grokked that, thanks :-)

// Update the title.
var titleText = g.selectAll('text.title')
.data(d3_chart_title);

This comment has been minimized.

@mbostock

mbostock Apr 8, 2011

Member

Since there's only one title per multiple, you can use g.select('text.title') here. As an additional benefit, you don't have to specify a data operator when you use select, since you'll automatically inherit the data from the parent node. That means you don't need to define the title attribute as an array on L36—you can just use a single property.

This comment has been minimized.

@jasondavies

jasondavies Apr 8, 2011

Collaborator

How do you add the initial title though? Or maybe you just detect if g.select(...) is empty?

This comment has been minimized.

@jasondavies

jasondavies Apr 8, 2011

Collaborator

Never mind, I think I'm confusing .select() with .filter() :-)

This comment has been minimized.

@jasondavies

jasondavies Apr 8, 2011

Collaborator

Okay I was right, but detecting emptiness is simple! See what you think.

.attr('class', 'title')
.attr("dy", "1em")
.style("font-weight", "bold")
.text(d3_chart_identity);

This comment has been minimized.

@mbostock

mbostock Apr 8, 2011

Member

If you use select rather than selectAll, then d3_chart_identity would be replaced with d3_chart_title (or d3_chart_bulletTitle).

Also, I often use String as shorthand for the identity function, as in text(String).

.attr("transform", "translate(120)");
var chart = g.selectAll('g.chart');

This comment has been minimized.

@mbostock

mbostock Apr 8, 2011

Member

Can use select for the chart and subtitle, too.

d3.chart.js Outdated
// Update the x-scale.
x1.domain([0, max]);

This comment has been minimized.

@mbostock

mbostock Apr 8, 2011

Member

If you want to reverse the scale, I would recommend something like this:

x1.domain([0, max]).range(reversed ? [width, 0] : [0, width]);

That way, you won't need your startpos or pos functions. Also, it means that if you flip the orientation, the bullet chart should do the correct transition from the old scale to the new scale! (You'll need to update x0 at the end in the same way. Another possibility is to create a new scale and then use assignment, x0 = x1.)

This comment has been minimized.

@jasondavies

jasondavies Apr 8, 2011

Collaborator

Yeah, I originally went the way of reversing the scale like that, but you can't have negative widths for rect so this minor setback made me create the *pos functions. Transitioning the flip sounds cool though, that's persuaded me to try it and work around the negative widths a different way :-)

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 8, 2011

Re. the "type" attribute, and single vs. double quotes, and conventions for naming functions: I'm quite OCD about all this stuff. I am sure you are right that my way is not necessarily the best way. :) But I do think consistency is important for readability, so I try to make everything look the same. It's difficult to force someone to conform precisely to my (pedantic) style, so I'm happy to do little style tweaks if you don't mind the merge overhead!

I would say re. function naming that using

function foo() { … }

is a bit more flexible than

var foo = function() { … };

mainly because the former is hoisted: it can be called before it's defined in the code. Whereas in the latter, the function itself is anonymous, and is not bound to the var foo until the line on which is defined.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 8, 2011

Ah, I wondered why you did that for functions. I'm trying to keep to double quotes for D3, but old habits die hard :-)

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 8, 2011

Do you have any thoughts on the "top" and "bottom" orientations? My plan is to just use transform="rotate(90)", if I do it right then maybe the transition will even rotate nicely!

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 8, 2011

I'm thinking of leaving the title and subtitle out of the chart implementation. It should be easy enough to plug that in after adding the chart type, and since the text is static I don't see a strong value in baking it into the chart. Plus, this way it avoids the hard-coded "translate(120)", allowing more flexibility in how the titles are displayed. Does that sound reasonable?

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 8, 2011

Sounds fine to me.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 8, 2011

Ah, there's a design problem here. We can't throw away the bullet chart instance because it is stateful—it's caching the old scale, x0. So if we throw the bullet chart away, then we lose the old scale and can't compute the transitions appropriately. Either we need to squirrel away the old scale on the outer svg:g node (which is icky), or we should change the bullet-multiples example to cache chart instances rather than recreating them on each transition.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 8, 2011

Hmm, the transitions do seem to work fine at the moment though, I think because they only rely on the new scale, x1 due to no new data being added (aside from the first time round). The x0 scale is only used to create new enter() nodes. So this would only affect the case of adding a new item (e.g. a new measure) and because x0 defaults to a domain of [0, Infinity] it would always appear to transition from zero, rather than from the original scale.

Let's say we fix this and use the original scale, and add a new measure that's out of bounds in the current scale, then it would immediately appear in the correct (out of bounds) location, and then transition along with the scale resizing to fit in the view box. With the current behaviour it would transition from zero. I'm not sure which behaviour is preferable, any thoughts?

I've come up with a cache example that works, let me know if you want me to push it...

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 8, 2011

You can see the bug in the transitions more easily with the ticks. It helps to slow down the duration, say to 5000 ms. For example, the first time you click "Randomize!" you'll see the new ticks for 900, 800, 700 and 600 appear at x=0, because the domain on x0 is reset to [0, ∞]. Similarly, if a new measure is added, I'd strongly prefer it being positioned initially using the old scale, even if that is out-of-bounds.

I've hacked it locally by binding the vis to an array of d3.chart.bullet instances rather than an array of orientations.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 8, 2011

Ah, yes, much more obvious at 5000ms! Adding the cache does indeed fix the problem, see my commit ccdbccf above.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 8, 2011

Yeah. I'm going to simplify the example a bit. It's nice that you can derive chart instances from data, but it will be much more common to just hard-code a particular orientation. Since people will likely use this as an example to derive their own visualizations it'll be good if we can keep it "dumb". Should have a commit incoming soon.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 8, 2011

Another interesting issue here is that sometimes you want the same scale across bullet charts, and other times you don't! In the initial data that we're using (in bullets.js) for example, each chart shows a different dimension, so there's no reason to force them to use the same scale. Probably this should be a flag on the chart instance, or perhaps we restore the ability to set the x-domain.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 8, 2011

Alright, I feel like this might be good to go? What do you think? Should we push the chart state into the element data, or keep it as-is? I made a feeble attempt at pushing it into the element data, but it was a bit more difficult than I expected; in some ways, maybe leaving it as chart state is more flexible?

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 8, 2011

Looks good to me! I'm undecided on what to do about the chart state, I think the way it works now is good enough though. In some ways, pushing it into the element data seems more correct as it means there is flexibility for the user to set the scale range. In the original Protovis layout, it was exposed as a maximum property. I'll sleep on it, but feel free to merge into master if you want.

The only other things are supporting "top" and "bottom" orientations, and custom marker symbols, but I guess they can be added later too. It's been fun :-)

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 9, 2011

Let me know if you've changed your mind after sleeping on it. Otherwise, I'll push to master.

And ya, lots of fun. :) Looking forward to subsequent chart templates!

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 9, 2011

Okay, having slept on it, I do think I prefer storing the chart state in element data, but I think it would be cleaner to store it on an inner svg:g element rather than the outer one. I've implemented this, but I'm using .selectAll even though there's just a single inner svg:g element so that I can set the data. For some reason I couldn't get it to work using .select, maybe I'll have another go or you can give me a hint :-)

Let me know what you think of using the inner node to store the state, I'm not too bothered but it does make the bullet-multiples example slightly nicer.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 9, 2011

OK, I updated the implementation to use g.each internally. I like doing this inside the chart implementation so that the chart can decide whether to use call or each. I removed computation of a shared scale across multiples.

The old scale is now stashed in a __chart__ field on the containing node. This is a bit of a hack but it seems like a reasonable place to hide old chart state solely for transitions. You're right that we could create nodes to store this information explicitly, even as data- attributes to serialize the state into the DOM! But it also seems reasonable to hide the state on the DOM and consider it semi-private state of the chart.

Potentially, the __chart__ field could be used for cross-type transitions, too. For example, you could replace a bar chart with a line chart and the bar chart would know the line chart's old scale. If we go that route, it'd be good to document the API of the __chart__ field more formally, but for now we could consider it "package private" (in Java terminology).

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 9, 2011

Also: I decided not to add enter to select.

I think it's best if the entering and exiting selection are limited to the data operator. There it's symmetric and it's more obvious that you need to apply the data operator in order to define the entering and exiting selection. And, that ties the definition of the entering and exiting selection more closely to the relational join between nodes and data.

Another reason select can't support the entering selection is that the selectAll operator currently determines the parent node for subsequent appends and inserts (group.parentNode). I'd have to change the implementation of entering selections to support append or insert into arbitrary nodes. That wouldn't be too hard, but it feels like the code is resisting this new feature so I think it's best to leave it out, at least for now.

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 9, 2011

Awesome, I love the idea of transitioning between chart types!

@mbostock mbostock closed this Apr 9, 2011

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 9, 2011

Good work! I refactored the bullet example for the website. Let me know if you want to make any additional changes!

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 9, 2011

Cool, thanks! I love collaborating in pull requests, it's really fun :-)

I've started working on the vertical orientations, I'll let you know when it's ready. I'm experimenting with using a rotate transform. Ideally the top-down order should be preserved as left-right order, which means the charts will cross over each other during a transition. Not sure if that'll look good but we'll see.

@mbostock

This comment has been minimized.

Member

mbostock commented Apr 9, 2011

Ya, I wouldn't worry too much about supporting transitions between orientations—that's going to be much less common than data updates. How will you handle text if you use a rotate transform?

@jasondavies

This comment has been minimized.

Collaborator

jasondavies commented Apr 9, 2011

Yeah I'd have to write some code in the example itself to reshuffle the text, so not ideal really. Hopefully using rotate will be simpler than lots of conditionals to switch between width/height and x/y for the chart itself though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment