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

Adds numeric value comparison filters #79

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

Conversation

MikeGodin
Copy link

Adds numeric value comparison filters for ">", "<", ">=", and "<=".

Added numeric value comparison filters for ">", "<", ">=", and "<=".
Added numeric value comparison filters
@bensheldon
Copy link
Member

@MikeGodin this looks great. Thanks for the PR!

The failing CI seems to be package rot isolated to node v0.8. We should be able to safely ignore it for this PR.

Mind if I ask how you're using Nodetiles?

@MikeGodin
Copy link
Author

@bensheldon, I'm using nodetiles to generate chloropleth maps based on data attached to US census block groups... hence the need for numeric filters.

if (feature.properties[filter.key] !== filter.val) {
return false;
}
break;
case "<":
if (feature.properties[filter.key] >= filter.val) {
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing the obvious, but why not use the same comparison operator that you’re handling? (e.g. do feature.properties[filter.key] < filter.val here because you are evaluating ">" instead of >=.) What you’ve got is technically equivalent, so it works, but makes reading it a bit confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, nevermind, that was a dumb comment. Ignore me :P

Copy link
Member

Choose a reason for hiding this comment

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

The filters work by returning early from the property loop based on a single false match, rather than iterating over the entire set of properties to ensure they all are true.

I don't know if it would be more performant to use a true functional method, but it's following the pattern of the previous implementation of the = matcher.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that’s what I realized right after posting. Too distracted this morning, I guess. Also been a while since I wrote this :P

I think encapsulating each of the kinds of tests in this method might be better for reading, though it would be slower. Either way, not really in the scope of work for this PR. My bad.

@Mr0grog
Copy link
Member

Mr0grog commented Jul 30, 2015

Can you add some tests? Otherwise, this looks pretty great to me.

@Mr0grog
Copy link
Member

Mr0grog commented Jul 30, 2015

(And thanks for contributing, @MikeGodin!)

@prashtx
Copy link

prashtx commented Jul 30, 2015

Not to detract from the awesome primary repo, but we also have some changes in LocalData/nodetiles-core that might be relevant/helpful for @MikeGodin. Most of our work there is considered "experimental", and I know there's at least one place we compromised on compatibility for performance, so we haven't done the work to port changes back to nodetiles/nodetiles-core. Sorry @bensheldon!

@Mr0grog
Copy link
Member

Mr0grog commented Jul 30, 2015

@bensheldon Should we just kill the 0.8 tests on Travis? Seems old enough now it might not be worth supporting. (By the way, I’ve noticed in several projects lately that NPM often has trouble resolving a workable set of dependencies for 0.8, so shrinkwrapping for the test environment might also be a good idea.)

@Mr0grog
Copy link
Member

Mr0grog commented Jul 30, 2015

@MikeGodin if you need any pointers on the tests, let me know. BUT it should be as simple as dropping three files into test/cartoRenderer:

Then, in test/cartoRenderer.spec.js, just add simpleTest("numericComparison") after line 13.

You don’t have to use the name “numericComparison” for all those. They just have to have the same name, whatever it is.

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.

4 participants