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

Add error returns to maxGridDiskSize #551

Merged

Conversation

isaacbrodsky
Copy link
Collaborator

This PR also changes the signature for this function to support k values where the number of cells is more than an int32_t can represent (i.e. k=26755, maxGridDiskSize(26755)=2147570341)

@coveralls
Copy link

coveralls commented Dec 14, 2021

Coverage Status

Coverage increased (+0.01%) to 98.185% when pulling f17615e on isaacbrodsky:error-returns-max-grid-disk-size into efad372 on uber:master.

@@ -23,7 +23,9 @@ H3Index pentagon = 0x89080000003ffff;

BEGIN_BENCHMARKS();

H3Index *out = malloc(H3_EXPORT(maxGridDiskSize)(40) * sizeof(H3Index));
int64_t outSz;
H3_EXPORT(maxGridDiskSize)(40, &outSz);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the benchmarks check for the error condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated for the setup part. I didn't feel the benchmark should check this as part of what it times since this isn't part of the code we're intending to measure.

Copy link
Collaborator

@dfellis dfellis left a comment

Choose a reason for hiding this comment

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

Only a minor comment on the validity of benchmarks that all fallible functions but assume correctness.

src/apps/testapps/testGridDisk.c Outdated Show resolved Hide resolved
src/apps/testapps/testGridDisk.c Outdated Show resolved Hide resolved
if (k < 0) {
return E_DOMAIN;
}
*out = 3 * (int64_t)k * ((int64_t)k + 1) + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to check somewhere that this value is less than the max number of cells at the current res? E.g. should kRing(res0cell, 100) throw E_DOMAIN?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a concern that in some cases this will overflow, in that case we should consider returning the maximum number of cells?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that's probably better than returning an error (and should be correct, in that I think k-rings covering more than the total number of cells should return all cells at that res, right?).

We should be able to check a constant to avoid integer overflow, right?

@ajfriend
Copy link
Contributor

ajfriend commented Jan 3, 2022

One minor comment: Should we standardize the type we use for distance? Here, it looks like we're using int, but we use int64_t in gridDistance.

DECLSPEC H3Error H3_EXPORT(gridDistance)(H3Index origin, H3Index h3,
int64_t *distance);

@isaacbrodsky isaacbrodsky merged commit a1157d8 into uber:master Jan 3, 2022
@isaacbrodsky isaacbrodsky deleted the error-returns-max-grid-disk-size branch January 3, 2022 22:35
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.

5 participants