Capped Row Chart is returning the BOTTOM N groups instead of the top N.

[grugknuckle]

I have this issue in dc.js v2.1.2 and I've created a block to show it.

I believe this is a bug in the capMixin for dc.js v2.1.2. Notice that the full chart has 20 rows. The TOP 5 rows by value are rows, 16 - 20. But when the row chart uses the .cap method of the capMixin, the BOTTOM 5 rows are displayed instead of the TOP 5 rows. The expected behaviour is to show the top 5 rows, not the bottom 5.

After looking at the source code for cap-mixin.js , I think the bug happens in lines 61-64.

if (_cap) {
    var start = Math.max(0, topRows.length - _cap);
    topRows = topRows.slice(start);
}

I can correct this issue locally by changing lines 62-63 to

 var end = Math.max(0, Math.min(_cap, topRows.length));
 topRows = topRows.slice(0, end);

But since I've only looked at how this affects the rowChart, I can't be sure there isn't a good reason why the code is the way it is. Also, there isn't a spec file for the cap-mixin, so maybe THAT is the actual bug.
Anyway, if I had to guess, I would say that this was fixed in an earlier version, but the problem came back when some merge was made.

EDIT: When I first posted this, I had transposed the min and max functions. The code above is correct now.

[gordonwoodhull]

Thanks for the report, @grugknuckle!

As of dc.js 2.1.2, group.top() is not used; specifically #1184 removed it from the capped mixin. We use the chart's ordering instead. (Previously the chart's ordering had to agree with the group's order, or you'd get incorrect results.)

But I agree that we may not have chosen the right end of the data, if you have set the ordering to show the largest values first.

And that's probably how most people set up their row charts, so I'm open to changing it.

It's tough improving the API of a tool that so many people use, with all its quirks.

cc @macyabbey

[grugknuckle]

@gordonwoodhull First of all, I want to say thanks for your hard work on managing this repository. I've been lurking behind the scenes and using dc.js since 2013 but I've never posted until today. I know that you and others have put some blood sweat and tears into it.

As for the functionality, I think the larger issue is that there isn't a spec for the cap-mixin which clearly tests what the expected output of chart.data() is supposed to be from the capMixin. If I'm wrong about that, please correct me. I suppose it could be hidden inside the core specs, but the specs folder didn't have a file that was obviously for the cap-mixin.

IMHO, the word 'cap' seems to mean take it off of the top. But perhaps, there should be some feature that allows the user to choose? Maybe that's a discussion for another time.

[gordonwoodhull]

Completely agree, @grugknuckle. In general, we only test the mixins via the charts - in other words, we have integration tests but not unit tests. Even where there are suites devoted to a mixin, I believe they only test the mixin through the charts derived from it.

The only tests for the cap mixin are currently in the pie chart specs. Some of the mixins would be difficult to test without a chart (since they implement drawing functionality) but this one can be isolated safely, since it's pure data and no rendering.^*

So I'm adding a cap-mixin-spec.js, as you suggest. And I independently came to the conclusion (before seeing your note) that there should be a takeFront flag, defaulted true, which people can set false e.g. if they want to sort their row/pie chart in ascending order and show the largest elements. So there will be, in 2.1.3.

^* In fact, I often wonder if the cap mixin should be removed from dc.js, since its functionality is so close to fake groups. The long-term goal is to abstract out all data processing into a data preparation library which sits between crossfilter and dc.js. But this should be done in a backward-compatible way, so the cap mixin will still be relevant.

[grugknuckle]

Thank you for your quick response to this @gordonwoodhull. I'll be downloading the bleeding edge build directly!

In fact, I often wonder if the cap mixin should be removed from dc.js, since its functionality is so close to fake groups.

That's a good point. I really hadn't thought about it that deeply. But now it occurs to me that I had actually attempted a work around to this issue with a fake group before looking closely at the source code.

The long-term goal is to abstract out all data processing into a data preparation library which sits between crossfilter and dc.js.

Ok, I wasn't aware of the long term vision, but I think this is even better. If the data could be capped prior to passing the group into the chart, then a lot of the headaches go away. That's all the cap mixin is really doing anyway.

[gordonwoodhull]

Right now I'm bogged down in fixing the pie chart tests, but you can check out the cap mixin tests here:

https://github.com/dc-js/dc.js/blob/hotfix/2.1.3/spec/cap-mixin-spec.js

(The cap mixin is complete too.)

Hopefully I'll have time to make the release today.

[gordonwoodhull]

fixed by 1f2463d and released in 2.1.3