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

Add function to get all base cells #176

Merged
merged 7 commits into from Jan 9, 2019

Conversation

Projects
None yet
5 participants
@zachasme
Copy link
Contributor

zachasme commented Jan 4, 2019

Resolves #174.

You could instead export a defined constant with all 122 base cells. What do you think?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 4, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 8539cfb on zachasme:feature-get-basecells into f844901 on uber:master.

@dfellis

dfellis approved these changes Jan 4, 2019

@isaacbrodsky
Copy link
Collaborator

isaacbrodsky left a comment

Thanks for the PR, I left a few comments.

Show resolved Hide resolved src/apps/testapps/testBaseCells.c Outdated
H3_EXPORT(getBaseCells)(baseCells);
t_assert(baseCells[0] == 0x8001fffffffffff, "correct first basecell");
t_assert(baseCells[121] == 0x80f3fffffffffff, "correct last basecell");
}

This comment has been minimized.

@isaacbrodsky

isaacbrodsky Jan 4, 2019

Collaborator

We should free baseCells, since it can help locate actual leaks using Valgrind a little easier.

Nit: we could use malloc since we don't need it zeroed, but that's definitely not a big deal.

* @{
*/
/** @brief provides all base cells */
void H3_EXPORT(getBaseCells)(H3Index *out);

This comment has been minimized.

@isaacbrodsky

isaacbrodsky Jan 4, 2019

Collaborator

I am wondering about the name of this function, since in the library a "base cell" usually refers to an integer identifier 0-122. Maybe getRes0Indexes or something?

* getBaseCells generates all base cells storing them into the provided
* memory pointer.
*
* @param out H3Index* the memory to store the resulting base cells in

This comment has been minimized.

@isaacbrodsky

isaacbrodsky Jan 4, 2019

Collaborator

We should document how large this buffer needs to be - if NUM_BASE_CELLS is needed, that perhaps should be in h3api.h.

This comment has been minimized.

@nrabinowitz

nrabinowitz Jan 7, 2019

Contributor

I'm not sure the JS build can currently access constants, even if they're in h3api.h. I think the version that would be most consistent with other pass-in-allocated-memory functions would be a new function res0IndexCount or similar, which would return NUM_BASE_CELLS.

@zachasme

This comment has been minimized.

Copy link
Contributor

zachasme commented Jan 6, 2019

Thank you for the comments, I've updated the pull request accordingly.

What are your thoughts on exporting a defined constant with the base cells instead?

@nrabinowitz

This comment has been minimized.

Copy link
Contributor

nrabinowitz commented Jan 7, 2019

Thanks for the submission! I personally prefer the function, particularly because the JS binding can't access constants directly (though as that's a binding issue we could resolve in the binding if the constant was a better option). Honestly though the generator function is likely so fast that there would be very little benefit to a constant.


SUITE(baseCells) {
TEST(getBaseCells) {
int count = res0IndexCount();

This comment has been minimized.

@nrabinowitz

nrabinowitz Jan 8, 2019

Contributor

This line needs H3_EXPORT(res0IndexCount) now.

This comment has been minimized.

@zachasme

zachasme Jan 8, 2019

Contributor

Good catch!

*/

#include <stdlib.h>
#include "constants.h"

This comment has been minimized.

@nrabinowitz

nrabinowitz Jan 8, 2019

Contributor

include still needed?

This comment has been minimized.

@zachasme

zachasme Jan 8, 2019

Contributor

Indeed not. I've removed it :)

@nrabinowitz
Copy link
Contributor

nrabinowitz left a comment

Looks good to merge, thanks!

@@ -6,6 +6,8 @@ The public API of this library consists of the functions declared in file
[h3api.h](./src/h3lib/include/h3api.h).

## [Unreleased]
### Added
- `getBaseCells` function for getting all base cells (#174)

This comment has been minimized.

@nrabinowitz

nrabinowitz Jan 8, 2019

Contributor

Whoops, this should now be getRes0Indexes (and you might want to add a note about res0IndexCount as well)

#include "test.h"

SUITE(baseCells) {
TEST(getBaseCells) {

This comment has been minimized.

@nrabinowitz

nrabinowitz Jan 8, 2019

Contributor

Nit: getRes0Indexes

@@ -275,6 +275,17 @@ double H3_EXPORT(edgeLengthM)(int res);
int64_t H3_EXPORT(numHexagons)(int res);
/** @} */

/** @defgroup getBaseCells getBaseCells
* Functions for getBaseCells

This comment has been minimized.

@nrabinowitz

nrabinowitz Jan 8, 2019

Contributor

Sorry, this too - getRes0Indexes

@zachasme zachasme force-pushed the zachasme:feature-get-basecells branch from 6a204c2 to 8539cfb Jan 9, 2019

@zachasme

This comment has been minimized.

Copy link
Contributor

zachasme commented Jan 9, 2019

I've rebased and addressed the comments.

@nrabinowitz

This comment has been minimized.

Copy link
Contributor

nrabinowitz commented Jan 9, 2019

Looks great. Thanks so much for adding this!

@nrabinowitz nrabinowitz merged commit d0a0860 into uber:master Jan 9, 2019

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment