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

Remove data by test function #11

Closed

Conversation

esjewett
Copy link
Member

@esjewett esjewett commented Jul 3, 2015

This addresses issue #6, allowing .remove() to take a test function.

@esjewett
Copy link
Member Author

esjewett commented Jul 3, 2015

Unfortunately this appears to have issues with using a test function when filters are in place. Looks like it removes both records outside the filter and records outside the test function. Need to investigate a bit.

@esjewett esjewett added this to the 2.0.0-rc.0.0 milestone Mar 16, 2016
@tannerlinsley
Copy link
Contributor

@esjewett have you investigated this any further?

@esjewett
Copy link
Member Author

@tannerlinsley Haven't gotten to it :-(

@nordfjord
Copy link

Hey guys, just wondering on the progress of this (opens up so many possibilities). I'm thinking about dedicating some time to this PR/issue over the weekend.

@esjewett You said in another thread that this approach was misguided have you figured out why?

@esjewett
Copy link
Member Author

@nordfjord No progress since we last updated this. If you can take a look, that would be excellent. You can see what I did in the referenced commits, but I think it would probably be best to start from scratch because those commits were done before the array dimension work. I don't remember why I said it was misguided, unfortunately. When I said in #6 that something was flawed about the approach, I just meant that it was not working but I didn't have a good handle on exactly why. If this was somewhere else, what I may have been getting at was this:

If we are going to support multiple simultaneous filters in any case (that can be switched between), then the remove function would just be updated to remove data in the context of a specific filter. We don't have an issue open for this multiple filter thing, but it has been discussed in issues on the original cross filter repo. In that way of looking at things, this change would be equivalent to creating a new filter myFilter based on filterFunction and then calling crossfilter.remove(myFilter), or whatever API we decide on around filters.

So maybe it would make sense to work on that first and this second, but I suspect that the work will be portable, so it won't be wasted to work on this function-based-removal problem directly.

@nordfjord
Copy link

I think the biggest challenge in this will be that different dimensions will need to remove different values.

Since if a dimension is filtered it only affects the groups of other dimensions which in turn means that a crossfilter wide remove needs to check if each dimension has already filtered an index since if it has we don't want to remove it.

@esjewett
Copy link
Member Author

This was fixed in #81. Woohoo!

@esjewett esjewett closed this Sep 22, 2017
@esjewett esjewett deleted the Remove-data-by-test-function branch September 22, 2017 16:05
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.

None yet

3 participants