Slightly improve invalid index error reporting#86
Merged
ajfriend merged 17 commits intouber:cythonfrom Dec 9, 2019
ajfriend:work
Merged
Slightly improve invalid index error reporting#86ajfriend merged 17 commits intouber:cythonfrom ajfriend:work
ajfriend merged 17 commits intouber:cythonfrom
ajfriend:work
Conversation
ajfriend
requested review from
ankitmehta21,
dfellis,
isaacbrodsky and
nrabinowitz
Codecov Report
@@ Coverage Diff @@
## cython #86 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 8 9 +1
Lines 165 173 +8
=====================================
+ Hits 165 173 +8
Continue to review full report at Codecov.
|
isaacbrodsky
reviewed
Dec 8, 2019
src/h3/util.pyx
Outdated
|
|
||
|
|
||
| cpdef H3int hex2int(H3str h): | ||
| cpdef H3int hex2int(H3str h) except *: |
Collaborator
There was a problem hiding this comment.
Per https://cython.readthedocs.io/en/latest/src/userguide/language_basics.html could except? 0 be used here?
Collaborator
Author
There was a problem hiding this comment.
good point! I'll change that
Collaborator
There was a problem hiding this comment.
Might also apply in other places like src/h3/util.pyx line 75, but that's not new in this PR.
Collaborator
Author
There was a problem hiding this comment.
Oooh, I totally didn't realize we had the except? syntax!
I'm now using cpdef H3int hex2int(H3str h) except 0, which should work, assuming that no valid H3 index can ever be zero.
In util.py, I'm thinking of using cdef H3int* create_ptr(size_t n) except? NULL, since NULL could potentially be a valid output if n = 0.
For the two examples...
edges.pyx,cpdef (H3int, H3int) edge_cells(H3int e) except *:geo.pyx,cpdef (double, double) h3_to_geo(H3int h) except *:
...I think we have to stick with the except *, as I can't figure out how to get the syntax to work with anything else for multiple return values.
I would think that multiple return values would evaluate to a tuple (which is a Python object) making the except * unnecessary, but that doesn't see to be the case (the tests expecting the error to appear fail). And something like except (0,0) doesn't seem to work either. Happy to change it if anyone knows a better way!
isaacbrodsky
approved these changes
Dec 9, 2019
nrabinowitz
approved these changes
Dec 9, 2019
|
|
||
| def num_hexagons(resolution): | ||
| return c.num_hexagons(resolution) | ||
| """ Return the total number of *cells* (hexagons and pentagons) |
Collaborator
There was a problem hiding this comment.
That's a good point - this should be numIndexes in the core lib in v4.
Collaborator
Author
There was a problem hiding this comment.
What about numCells? An "index" could still be a unidirectional edge, right?
Collaborator
There was a problem hiding this comment.
I think there's some general terminology we ought to work out for v4... currently we've been using index consistently for cell index, but it might be worth refining.
ajfriend
added a commit
that referenced
this pull request
May 10, 2020
* Use Hunter
* Use position independent code to link shared object correctly
* Fix issues in module naming
* Temporary fix
* Use src directory and fix CMake build for subdirectory
* Fix build, remove dependency on Hunter
* aj first stab at cython
* Added class to manage H3Index array memory.
Added a bunch of wrapping functions.
* hacky version of polyfill working
* remove some naming redundancy
* Adding compact and uncompact functions
* notes
* clean up HexMem class
* reworked geofence and geopolygon. now memorysafe (i think)
* reorder definitions
* formatting
* Use scikit-build for setup
* Changes for getting build partially working on macOS
* Add scikit-build to requirements-dev.txt
* Build the core library as static and update notes.md
* update cython notes
* Cleaning up the Cython instructions and tests a bit
* cleaning out extra files for now
* readme
* Cleaning out more files to focus on Cython dev in this branch
* makefile
* simplify install process with makefile
* adding back in a bunch of files
* Add appveyor.yml for Cython
* Disable Python 2.7 test
* reorganize cython build; working on os x
* get new changes running on my OSX machine
* add hexmem.pyx
* just throwing it all in h3core...
* newlines
* separate HexMem into separate module
* change import to `h3py.h3core` for tests
* update readme for new folder structure
* starting to layout separate APIs
* `set_int` and `set_str` working
* notes
* cleaning up a bit
* moving the HexMem helper functions
* moving lat/lng functions to separate module
* move output formatting functions
* use `size_t` in HexMem
* trying input/output formatters
* rename to make api modules obvious
* adding some functions providing hexagon stats
* adding some TODOs to use C functions instead of python
* fix api typos
* adds unidirectional edge functions
* cleaning up geo functions a bit
* add todo
* create `api` directory and move modules
* try using `_api_template.py` to generate different APIs
* remove HexMem class and use memoryviews of cython.view.array objects instead
* make single mercator function
* remove redundant exception handling
* automatic input validation
* add validation for unidirectional edges
* simplify API generation
* a little cleaning
* defaults for parent and child functions
* add back in H3str as input type to hex2int
* simplify validation and conversion code
* adding tests for geo functions
* rename hexmem -> util
* move exceptions to util.pyx
* move geo functions to a separate module
* move hex2int and int2hex to each api
* rename validation functions
* rename h3api -> libh3 to reserve "api" for the different python APIs we expose
* cleaning up memory allocation
* fix clean up code
* clean up comments
* add some resolution checking
* adding a diagram of the code to remove zeros
* adding some docstrings
* update H3 C library
* add line function
* docstring
* some function name changes
* rename k_ring -> disk
* rename hex_ring -> ring
* cleaning up modules
* ... cleaning up all the modules
* add in backward-compatible `old` API
* add some thoughts on slower fallback for `ring`
* some naming changes
* fix up mean_hex_area and mean_edge_length
* rename base_cell -> get_base_cell
* remove special case for n=0 memory allocation
* rename edge_hexes -> edge_cells
* rename edges_from_hex -> edges_from_cell
* rename functions to match current python implementation
* remove h3py.api.old in favor of defaulting to set_str
* update resolution docstring
* use macro in CMake (I have no idea what I'm doing tho...)
* try this one weird trick for Python 2 compatibility!
* add note on .rstrip('L') for handling python 2
* Use `__version__.py` (#57)
* use __version__.py to define python version
* add Python/C version check test
* newline
* some questions on the CMake file
* actually, use _version.py for less confusing naming
* note
* Change directory structure, make tests work (#61)
* travis with multiple python versions (#62)
* Add appveyor.yml for Cython
* Disable Python 2.7 test
* Use `__version__.py` (#57)
* use __version__.py to define python version
* add Python/C version check test
* newline
* some questions on the CMake file
* actually, use _version.py for less confusing naming
* note
* Change directory structure, make tests work (#61)
* linting
* linting
* fix purge command
* clean up requirements a bit
* messing with travis
* more travis
* try travis again
* clean up requirements a bit more
* test more python versions, including 2.7
* cmake version thing
* py3.5 doesn't seem to work with cmake
* run all tests in test directory
* remove fabfile
* make sure fails on lint
* get lint to pass
* fix purge command
* bare bones requirements-dev.txt
* use `pytest tests/*` in CI configs
* no need for a `MANIFEST.in` anymore
* fix windows wildcard?
* whatever you want, Windows.
* Basic code coverage (#63)
* basic code coverage
* rename error types and drop prefix on h3ilb.H3int
* Rework some internal names (#65)
* rename `h3utils` to `util`
* typo in cmakelists
* rename `h3_core` to `memview_int` to match format of other interfaces
* linting passes on tests
* make sure travis lints the tests
* getting better coverage on the interfaces
* newline
* add `array_int` (numpy) tests
* install numpy
* make sure appveyor is running all the tests
* dev readme changes
* setup.py changes
* more setup.py changes
* Polyfill working on polygons with holes (#71)
* jupyter lab makefile target for testing
* got polyfill with holes working
* linting
* docstring for h3_to_geo
* improve coverage
* ignoring more flake rules, whilst listening to one
* make polyfill resolution a required positional argument
* use boolean lnglat_order instead of string
* move polyfill tests to own file
* test polyfill polygon input permutations (clockwise vs counterclockwise elements and closed or open loops)
* update description of polyfill inputs
* comment on test
* lint things up
* add some test comments
* don't need to check for null before free
* Add `h3_set_to_multi_polygon` (#74)
* got to_multipolygon workin
* clean up code and add a simple test
* don't need this import
* moved the to_multipoly stuff to own file
* got the geo_json bit working
* add some tests
* linting
* file rename
* update renaming thoughts
* clean up usage of H3int
* no globs!
* don't need this "mercator" business
* rename `check_addr` to `check_cell`
* `destroyLinkedPolygon` note
* rename geo2coord to deg2coord and coord2geo to coord2deg
* Get original test suite working (#79)
* got to_multipolygon workin
* clean up code and add a simple test
* don't need this import
* moved the to_multipoly stuff to own file
* got the geo_json bit working
* add some tests
* linting
* file rename
* update renaming thoughts
* clean up usage of H3int
* getting some more of the original tests to work
* get a bunch of tests working!
* got some more tests working
* got all but one test passing; modified some tests that no longer need to fail
* update test, based on what I think the right result is
* update test for uni edge
* all linty fresh now
* split `out_collection` into `out_ordered` and `out_unordered`
* upgrade to h3core 3.6.1
* bump version
* add notes on empty array inputs
* 100% code coverage (#82)
* add int/str conversion test
* 100% codecov!
* linting
* remove notes
* to `not` or not to `not`
* gotta have a badge!
* Get tox working (#83)
* use relative imports
* clean up some messages
* simplify dynamic api modules a bit
* making some adjustments for tox
* update appveyor script
* remove linux wheel-building code, as i think it is out of date
* fix edge_is_valid test
* linting
* add some badges
* mess with travisci a little bit
* more messing with travis
* flake8 issue
* continue trying to fix flake issue
* we'll get it eventually
* messing with cmake
* revert the travis.yml file; add flake8 to tox
* and revert cmake pin
* messing with appveyor
* revert appveyor change. not sure how to wildcard in appveyor
* separate flake8 in tox envs
* move `cell` and `edge` functions to separate files
* have api import almost everything from core.py
* get outta here, flake8!
* clean up `_api_template.py`
* clean up int -> hex conversion function
* notes on `move_nonzeros()`
* more notes
* refactor so that all cython code is used via `h3.core`
* move noqa
* Convert original test suite to use pytest syntax (#84)
* convert original test suite to use pytest syntax
* making some key hexes obvious
* have travis print the full stack trace on error
* make a few more hex keys obvious
* rework test_compact_malformed_input a bit
* lint!
* formatting
* adding back in a few comments
* move uses of `map` to list comprehensions
* Slightly improve invalid index error reporting (#86)
* slight improvement to invalid index error reporting
* linting
* install notes
* parallel tox doesn't seem that much faster
* use relative imports
* hide h3.core from user api namespace
* noqa on __init__.py
* hides internal modules, but seems to break tox
* remove the testing notebook
* revert files back
* rename `core` to `internal_api`
* add the H3 error types to the public API (for testing)
* add a few function docstrings
* rework errors in tests
* address some comments
* have `create_ptr` do an `except? NULL`, which should be faster than `*`
* version docstring
* Get _cy folder layout working (#87)
* try to get _cy folder layout working
* Fix CMake build
* specify the LIBRARY DESTINATION to be the _cy folder
* move `_internal_api` to `_cy/__init__.py`
* remove version checks in CMakeLists.txt (#88)
* Better API names (#89)
* rename APIs
* rename test files
* remove obsolete comments
* update test files in appveyor
* Use `basic_` prefix instead of `py_`
* makefile
* appveyor!
* API docs
* linting
* tweak docstrings
* missed one!
* Immutability questions for the future
* linting
* module docstrings have to go first
* clarifying API collection input expectations
* input collection tests
* Add more docstrings to the APIs (#91)
* new docstrings
* use pandas docstring guide formatting for the first few functions
* Adding a few more docstrings
* addressing comments
* taking care of comments
* use `H3Cell` instead of `H3Index` to be more precise
* Update h3_get_base_cell
* `h3_is_res_class_III` docstring
* spacing
* move todos to comments
* notes on collections
* module docstring
* polyfill and to_multi_polygon docs
* clean up some docstrings
* address comments
* Segfaults (#92)
* making some segfault notes on `polyfill`
* Allow Cython to handle the error
* notes on tests
* rename test to `test_test_invalid_polygon` and group into single test
* typo
* Add `get_pentagons` function (#95)
* add `get_pentagons`
* linting
* rename to `get_pentagon_indexes`
* Prep cython branch readme before merging cython into master (#98)
* needs more badges!
* prep the readme before merge to master
* add logo
* remove docs folder
* use hex notation in the integer interface test
* conforming to the tyranny of `E222 multiple spaces after operator`
* adding long description to setup.py
* lint
* python 2.7 compatibility
* cell/edge boundary: clean up geo_json branch
* describe why we use `hex` in the error messages instead of `int2hex`
* geofence free notes
* update changelog
* typo
* alg -> algorithm
* remove irrelevant todo
* add distance >= 0 error checking
* add a missing file to appveyor tests
* trying out appveyor
* why won't you listen to reason, appveyor!?
Co-authored-by: Isaac Hier <ihier@uber.com>
Co-authored-by: Isaac Hier <isaachier@gmail.com>
Co-authored-by: Isaac Brodsky <isaac@uber.com>
Co-authored-by: Isaac Brodsky <isaac@isaacbrodsky.com>
Co-authored-by: Kurt Smith <kusmith@homeaway.com>
Co-authored-by: ankitmehta21 <ankitmehta@uber.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pip installfrom the github repo to test things out!next step
_cyfolder. However, I can't quite get that working with Cython/CMake/scikit-build, so I'll put up a (failing) PR with the basic structure I'd like and see if I can get some help to get it working.