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

Decrease filter step size for small domains #958

Merged
merged 3 commits into from
Mar 9, 2020

Conversation

isaacbrodsky
Copy link
Contributor

Otherwise the detected filter step size may be larger than the domain,
which results in not being able to use the filter. This is applicable
when working with small percentages (<0.1%).

While changing this I also noticed this will treat negative numbers as
very small; I changed getNumericStepSize to compute the step size
based on absolute value. (Not sure the UI fully supports selecting negative
filter ranges, seemed like it wouldn't let me select them via mouse.)

Otherwise the detected filter step size may be larger than the domain,
which results in not being able to use the filter. This is applicable
when working with small percentages (<0.1%).

While changing this I also noticed this will treat negative numbers as
very small; I changed `getNumericStepSize` to compute the step size
based on absolute value.

Signed-off-by: Isaac Brodsky <isaac@isaacbrodsky.com>
} else if (diff <= 1) {
// Try to get at least 100 steps - and keep the step size below that of
// the (diff > 1) case.
const x = diff / 100;
Copy link
Contributor

@heshan0131 heshan0131 Feb 20, 2020

Choose a reason for hiding this comment

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

can you make this min step to be 1000? currently when 1 < diff < 3 step = 0.001, and when diff = 1, step = 0.01

Copy link
Contributor Author

@isaacbrodsky isaacbrodsky Feb 24, 2020

Choose a reason for hiding this comment

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

Sure, updated. I also updated tests based on the values I saw

edit: I still have an issue in CI (https://travis-ci.com/keplergl/kepler.gl/jobs/290743951) but I don't get that error locally - any ideas?

Signed-off-by: Isaac Brodsky <isaac@isaacbrodsky.com>
@isaacbrodsky
Copy link
Contributor Author

From the Travis CI runs, it looks like the results are not consistent between Node 8 and Node 10. Is there an assertion utility for asserting "floating point equal within some epsilon"?

@heshan0131
Copy link
Contributor

you can try use import {preciseRound} from 'utils/data-utils' to compare the result?

Signed-off-by: Isaac Brodsky <isaac@isaacbrodsky.com>
@isaacbrodsky
Copy link
Contributor Author

Seems the tests are passing now although I didn't update them to use preciseRound (Would have needed to be change in cmpFilters in comparison-utils, potentially among others)

@heshan0131 heshan0131 merged commit f35c403 into keplergl:master Mar 9, 2020
heshan0131 added a commit that referenced this pull request Mar 9, 2020
@isaacbrodsky isaacbrodsky deleted the filter-step-size branch March 9, 2020 23:26
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