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

#934 - Removing top from cap mixin #1184

Closed
wants to merge 1 commit into from

Conversation

macyabbey
Copy link

Before, the cap mixin used crossfilter.group.top(_cap) before applying the ordering function. This produced really strange results when you use a non-default ordering function.

The order of group.all by default is ascending value. The order of dcjs's ordering function is ascending key by default. So if you left ordering default, you would get fairly expected results. The largest value groups, sorted by key.

But if you use your own ordering function, you get very strange results, because your ordering function does not take affect until the biggest set has already been determined by group.all().top().

Added a new test which fails with current capping logic because of this problem. Changed implementation of data function in cap which resolves this new test. Changed existing tests which tested for incorrect results that used to be produced.

…fore applying the ordering function. This produced really strange results because of how the top function works. It uses crossfilter.group.all(), and then takes the last N elements as the top. The order of all by default is ascending value by default. The order of dcjs's ordering function is ascending key by default. So if you left everything default, you would get fairly expected results. The largest value groups, sorted by key. But if you use your own ordering function, you get very strange results, because your ordering function does not take affect until the biggest set has already been determined by group.all().top(). Added a new test which fails with current capping logic because of this problem. Changed implementation of data function in cap which resolves this new test. Changed existing tests which tested for incorrect results that used to be produced.
@gordonwoodhull
Copy link
Contributor

Thanks @macyabbey, this is great. It's been something we've been talking about for a while so I'm glad to see someone take the plunge.

I think it is a breaking change even though it's hard to argue that anything was good about the old behavior. So I think this will need to go into the 2.1 (develop) branch rather than 2.0.

I'd be glad to hear arguments either way.

@gordonwoodhull gordonwoodhull added this to the v2.1 milestone Jul 22, 2016
@macyabbey
Copy link
Author

@gordonwoodhull I agree this is a breaking change. What do you think about updating the cap documentation? I think we should make it a lot more clear about how the cap mixin interacts with the ordering function, and the underlying crossfilter group order function.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jul 25, 2016

Sounds like the right way forward - update the documentation for 2.0 with a note saying basically "We know this stinks but here is the way it currently works: group.order then top(K) then baseMixin.ordering. This behavior is deprecated and will change in 2.1."

Would you be willing to take a stab at that in a separate PR?

Both branches are active, so we could commit both at once.

gordonwoodhull added a commit that referenced this pull request Jan 13, 2017
the comment was mostly describing invariants for replacing group.top()
with group.all() + computeOrderedGroups + slice (9d07086)

slice's second parameter has a reasonable default
for #1184
* ascending but the N biggest where N is cap.
*
* Since computeOrderedGroups is using crossfilter's quicksort, the
* sorted items will always be in ascending order.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not always, see #1269

gordonwoodhull added a commit that referenced this pull request Jan 30, 2017
fixes #1296

s/value.value/value.total/ because wot
use describe blocks with beforeEach for each configuration
use -kv.value ordering for a test or two, it's the normal thing to do
takeFront is now the default so that broke a lot of the changes from #1184

group order no longer matters but retain a test showing how -kv.value is
equivalent

at least two tests made no sense before or after #1184, hopefully they
do now
@gordonwoodhull
Copy link
Contributor

Merged in 2.1.3 and the option to take from either the front or the back of the sorted array added in 2.1.4.

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

Successfully merging this pull request may close these issues.

2 participants