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

Cell-specific area calculations #377

Merged
merged 143 commits into from
Sep 27, 2020
Merged

Cell-specific area calculations #377

merged 143 commits into from
Sep 27, 2020

Conversation

ajfriend
Copy link
Contributor

@ajfriend ajfriend commented Aug 3, 2020

Adds functions to compute the exact spherical area of individual H3 cells:

  • cellAreaRads2
  • cellAreaKm2
  • cellAreaM2

Also exposes to the public API some related functions for computing
haversine (great circle) distance between spherical points, and the
lengths of H3 unidirectional edges:

  • pointDistRads
  • pointDistKm
  • pointDistM
  • exactEdgeLengthRads
  • exactEdgeLengthKm
  • exactEdgeLengthM

@coveralls
Copy link

coveralls commented Aug 3, 2020

Coverage Status

Coverage increased (+0.04%) to 99.187% when pulling a0ccdef on ajfriend:area into f8bbc4b on uber:master.

@ajfriend
Copy link
Contributor Author

ajfriend commented Aug 8, 2020

@ajfriend
Copy link
Contributor Author

In working on this, I've got a couple of questions for the group.

  1. Does it make sense to expose functions like these to users?
double H3_EXPORT(pointDistRads)(GeoCoord a, GeoCoord b);
double H3_EXPORT(pointDistKm)(GeoCoord a, GeoCoord b);
double H3_EXPORT(pointDistM)(GeoCoord a, GeoCoord b);

(I'm using pointDist* since GeoCoord will change to GeoPoint in 4.0.)

I'm thinking it does, since the area, length, and bounding box algorithms all depend on a "great circle" distance calculation. Also, this calculation depends on our choice for model of the earth, the earth radius we're using, and the distance formula we've chosen. Exposing this to users could help them debug things when numbers don't seem to add up quite right.
Also, exposing the Rads version makes it easier to use for spherical bodies other than the Earth.

  1. I think it would make sense to make sure that every downstream function uses one of the three listed above, to ensure everything in the library is using the same distance logic.

  2. Does it make more sense for these functions to be pass-by-reference, or pass-by-value?

    • Pass-by-value seems a little cleaner/safer, but the library seems to prefer pass-by-reference for structs (to reduce data on the stack, I assume).
    • However, GeoCoord objects are pretty small (just two doubles), so I'm not sure the data on the stack concern is as relevant.
    • Curious to learn how we've thought about this in the past!

@ajfriend
Copy link
Contributor Author

Very likely, the answer will be "Just use pass-by-reference", but I wanted to make sure that it still made sense, rather than going with it blindly.

Copy link
Collaborator

@nrabinowitz nrabinowitz left a comment

Choose a reason for hiding this comment

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

Super exciting to see these come together!

@ajfriend
Copy link
Contributor Author

I implemented and tested 3 different distance functions:

  • _pointDist_lawOfCosines
  • _pointDist_mystery
  • _pointDist_haversine

They seem to be numerically indistinguishable; they each fail the cellArea test somewhere between a tolerance of 1e-6 and 1e-7.

However, I'm not sure if there's a performance benefit of one over the others (and this might also be affected by our choice ov pass by reference vs value.)

@ajfriend
Copy link
Contributor Author

@@ -260,6 +275,16 @@ double H3_EXPORT(hexAreaKm2)(int res);

/** @brief average hexagon area in square meters (excludes pentagons) */
double H3_EXPORT(hexAreaM2)(int res);

/** @brief exact area for a specific cell (hexagon or pentagon) in radians^2 */
double H3_EXPORT(cellAreaRads2)(H3Index h);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume for these functions they have undefined output when the H3 index is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, should we document that? or test for validity?

@ajfriend ajfriend merged commit 2c41e74 into uber:master Sep 27, 2020
@ajfriend ajfriend deleted the area branch September 27, 2020 03:04
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
* translating over my python/cython code

* just testing

* revert

* try exporting the cell area function

* adding a simple test

* a few extra functions

* exposing great-circle/haversine distance function to user

* messing around with everything computing distances!

* haversine is a bit better!

* numerical tests

* function with true haversine formula

* remove old note

* move to pass by reference

* translation bug in _pointDist_mystery

* remove _geoDistRads and _geoDistKm in favor of new user-facing functions

* spelling

* adding todo's just so i can highlight the function

* more todo!

* refactor the area computation a bit

* drop the todos

* adding unidirectional edge length functions

* add .ipynb_checkpoints to .gitignore

* remove alternate distance functions

* remove constrainLat and constrainLng

* Revert "remove constrainLat and constrainLng"

This reverts commit 0962949.

* rads -> meters -> kilometers

* tests workin!

* formatting thing?

* tests for the edge length function

* bug?

* valgrind bugfix

* valgring bugfix attempt uber#2

* valgrind bugfix uber#3

* much cleaner tests

* remove old tests

* bug

* edge iterator function to clean up tests

* edge length tests at several resolutions

* refactoring the Earth area test

* tests for each group of units

* a broken test at resolution 1

* positivity tests

* area bug coming from not handling the zero elements in uncompact

* area tests up to res 4

* tests for pointDistM

* tests for pointDist* functions

* documenting packNonzeros

* move testing code to src/apps/testapps/testH3CellAreaExhaustive.c

* meh

* headers not needed

* add some test documentation

* more function/algorithm notes

* more function notes

* getCellsAtRes func documentation

* fix docs in h3api.h.in

* dummy push because valgrind failed last time

* update changelog

* notes on `make test-fast`

* try sudo apt update to fix issue with valgrind

* use floats for division

* addressing some formatting comments

* ordering of measurements in terms of units

* clean up some long lines

* typo

* translating over my python/cython code

* just testing

* revert

* try exporting the cell area function

* adding a simple test

* a few extra functions

* exposing great-circle/haversine distance function to user

* messing around with everything computing distances!

* haversine is a bit better!

* numerical tests

* function with true haversine formula

* remove old note

* move to pass by reference

* translation bug in _pointDist_mystery

* remove _geoDistRads and _geoDistKm in favor of new user-facing functions

* spelling

* adding todo's just so i can highlight the function

* more todo!

* refactor the area computation a bit

* drop the todos

* adding unidirectional edge length functions

* add .ipynb_checkpoints to .gitignore

* remove alternate distance functions

* remove constrainLat and constrainLng

* Revert "remove constrainLat and constrainLng"

This reverts commit 0962949.

* rads -> meters -> kilometers

* tests workin!

* formatting thing?

* tests for the edge length function

* bug?

* valgrind bugfix

* valgring bugfix attempt uber#2

* valgrind bugfix uber#3

* much cleaner tests

* remove old tests

* bug

* edge iterator function to clean up tests

* edge length tests at several resolutions

* refactoring the Earth area test

* tests for each group of units

* a broken test at resolution 1

* positivity tests

* area bug coming from not handling the zero elements in uncompact

* area tests up to res 4

* tests for pointDistM

* tests for pointDist* functions

* documenting packNonzeros

* move testing code to src/apps/testapps/testH3CellAreaExhaustive.c

* meh

* headers not needed

* add some test documentation

* more function/algorithm notes

* more function notes

* getCellsAtRes func documentation

* fix docs in h3api.h.in

* dummy push because valgrind failed last time

* update changelog

* notes on `make test-fast`

* use floats for division

* addressing some formatting comments

* ordering of measurements in terms of units

* clean up some long lines

* typo

* updating copyright to 2020 in a few files

* few more files

* and just a few more

* testing area calculations on a few specific cells

* formatting

* working tests!

* add cellArea defgroup

* relax error tolerance for Windows... :/
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