Skip to content
This repository was archived by the owner on Oct 30, 2021. It is now read-only.

[testing] Remove prefiltering#131

Closed
springmeyer wants to merge 2 commits intomasterfrom
remove-pre-filtering
Closed

[testing] Remove prefiltering#131
springmeyer wants to merge 2 commits intomasterfrom
remove-pre-filtering

Conversation

@springmeyer
Copy link
Contributor

@springmeyer springmeyer commented Sep 26, 2018

This reverts part of #126 because I'm worried it could be, despite all unit test passing here in carmen-cache, cause a regression in results. The concern would be that the prefiltering throws out results which might not be thrown out after the 40 context limit later on in the coalescing.

The original motivation of this change was to increase performance by reducing the number of results that needed to be sorted internally.

This PR is intended to help test whether this change could potentially be a source of regressions in desirable results so we can have quick data to consider the tradeoffs of perf and results quality.

@springmeyer
Copy link
Contributor Author

It sounds like, from @ingalls and @apendleton, this change was not a cause for bugs downstream. So, closing this.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant