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

FIX-#431: implement sampling threshold and edit tests and docs #432

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

westernguy2
Copy link
Contributor

Signed-off-by: Kunal Agarwal kagarwal2@berkeley.edu

Overview

I removed the sampling_cap and sampling_start from the config and execute_sampling. Instead, I added a sampling_thresh, which is a threshold for which we'd begin sampling. This threshold forces any data larger than the threshold to sample the data such that the sample is equal to the size specified by the threshold. More information can be found in the documentation that was edited as part of this PR.

Changes

I edited config and execute_sampling to implement this fix. I also edited the tests to account for the removal of the old configs and the addition of the new one. I finally edited the documentation to reflect these changes.

Example Output

The issue described in #431 should be fixed now.

Signed-off-by: Kunal Agarwal <kagarwal2@berkeley.edu>
lux/_config/config.py Outdated Show resolved Hide resolved
tests/test_maintainence.py Outdated Show resolved Hide resolved
@dorisjlee
Copy link
Member

dorisjlee commented Nov 13, 2021

The code changes look great. It seems like test_maintainence.py is failing because the 10k row sample is probably not sufficient for these use cases. The sampling can be problematic for Similarity or Filter based actions. In this case, given that the intent involves filter, the Similarity action iterates over all the line charts on each filter.

Signed-off-by: Kunal Agarwal <kagarwal2@berkeley.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants