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 h3IsValid false positives on certain invalid strings, bump core library to 3.6.3 #81

Merged
merged 5 commits into from
Jan 23, 2020

Conversation

nrabinowitz
Copy link
Collaborator

Fixes #79

This diff has an appreciable impact on h3IsValid benchmarks:

master

h3IsValid x 4,047,006 ops/sec ±1.14% (81 runs sampled)

this branch

h3IsValid x 3,314,650 ops/sec ±0.85% (87 runs sampled)

However I can't find a more performant approach that fixes the false positives, which stem from parseInt ignoring unparsable chars. Open to suggestions. The perf is still very good, obviously, but this could be an issue if consumers need to validate large sets of hexagons.

@coveralls
Copy link

coveralls commented Jan 21, 2020

Pull Request Test Coverage Report for Build 174

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 170: 0.0%
Covered Lines: 363
Relevant Lines: 363

💛 - Coveralls

CHANGELOG.md Outdated
### Fixed
- Fixed `h3IsValid` returning true on certain edge cases (#81)
### Changed
- Updated the core library to 3.6.3 (#81)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: do we want to mention what was changed in the core library?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I say unless we come up with a good way to auto-populate that, it's a bunch of make-work and not super valuable (those interested can look at the core library CHANGELOG).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a short note. I think it's worthwhile if the changes are substantive, since most consumers will only look at the binding notes and may not realize they need to look further to understand changes.

@nrabinowitz nrabinowitz merged commit be3a542 into uber:master Jan 23, 2020
@nrabinowitz nrabinowitz deleted the invalid-strings branch January 23, 2020 21:08
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.

h3IsValid returns true on invalid H3 indexes
4 participants