Skip to content

Change directory structure, make tests work#61

Merged
ajfriend merged 1 commit intouber:cythonfrom
ankitmehta21:cython-ankit
Sep 24, 2019
Merged

Change directory structure, make tests work#61
ajfriend merged 1 commit intouber:cythonfrom
ankitmehta21:cython-ankit

Conversation

@ankitmehta21
Copy link
Contributor

  • Setup up the following structure:
h3-py/
  src/
    h3/
    h3lib/
  tests/
  setup.py
  ...
  • Reorganize export structure so that library name is h3 instead of h3py

@CLAassistant
Copy link

CLAassistant commented Sep 19, 2019

CLA assistant check
All committers have signed the CLA.

@ankitmehta21 ankitmehta21 force-pushed the cython-ankit branch 2 times, most recently from 3c14fd7 to f51ffa4 Compare September 19, 2019 00:46
@ajfriend
Copy link
Collaborator

Awesome! Thanks for this!

Can you make it so that the files are logged in Git as moved, instead of deleted and re-created? That would make any changes easy to see in this diff, and preserve the file change history, along with the reasons behind historical changes.

@ajfriend ajfriend self-requested a review September 19, 2019 17:57
@dfellis
Copy link
Collaborator

dfellis commented Sep 19, 2019

Awesome! Thanks for this!

Can you make it so that the files are logged in Git as moved, instead of deleted and re-created? That would make any changes easy to see in this diff, and preserve the file change history, along with the reasons behind historical changes.

Git doesn't have that ability. Any git mv is actually just a delete and create internally, and the git diff tool uses heuristics to figure out if it was moved or not.

In this case, Ankit's changes were significant enough to break the heuristics. Unfortunately you'll have to pull this branch locally and diff file1 file2 yourself to compare them.

@ajfriend
Copy link
Collaborator

Also, @ankitmehta21, did you base this diff off of the current uber:cython branch? The Cython files look like they're from an older version.

I think that may be why I was seeing all the files as deleted/re-created above :)

@ankitmehta21 ankitmehta21 force-pushed the cython-ankit branch 5 times, most recently from cf8caa1 to 45d0af1 Compare September 23, 2019 19:46
@ajfriend
Copy link
Collaborator

Maybe just a nit, but would it make sense to pick one of libh3 or h3lib, and use that single name throughout the codebase. I think we currently have src/h3lib/, but have src/libh3.pxd.

I don't have a preference for either, other than that they be consistent, if that's possible.

@ankitmehta21 ankitmehta21 force-pushed the cython-ankit branch 2 times, most recently from 3e802d4 to 1ecde8c Compare September 23, 2019 20:49
@ankitmehta21
Copy link
Contributor Author

Maybe just a nit, but would it make sense to pick one of libh3 or h3lib, and use that single name throughout the codebase. I think we currently have src/h3lib/, but have src/libh3.pxd.

I don't have a preference for either, other than that they be consistent, if that's possible.

Resolved, also I played around with the config to get the windows builds to pass. Thanks @dfellis for the pointers!

@ajfriend
Copy link
Collaborator

I'm getting an error when cloning and installing on my local machine:

[ajfriend@ajfriend-C02S63FKG8WM ~/work/temp/h3-py] (cython-ankit)
$ make init
find src -type d -name '*.egg-info' | xargs rm -r
find . -type f -name '*.pyc' | xargs rm -r
find . -type d -name '*.ipynb_checkpoints' | xargs rm -r
git submodule update --init
virtualenv -p python3 env
Running virtualenv with interpreter /usr/local/bin/python3
Using base prefix '/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7'
/usr/local/lib/python2.7/site-packages/virtualenv.py:1047: DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
  import imp
New python executable in /Users/ajfriend/work/temp/h3-py/env/bin/python3.7
Also creating executable in /Users/ajfriend/work/temp/h3-py/env/bin/python
Installing setuptools, pip, wheel...done.
env/bin/pip install -r requirements-dev.txt
Looking in indexes: https://yoober8:****@pypi.uberinternal.com/index, https://pypi.python.org/simple
Collecting cython (from -r requirements-dev.txt (line 2))
  Using cached https://pypi.uberinternal.com/packages/62/13/2ed2e2005afb114ffe08fbd25d890e3795ca86cebf11c97ff8877da58f28/Cython-0.29.13-cp37-cp37m-macosx_10_6_intel.macosx_10_9_intel.macosx_10_9_x86_64.macosx_10_10_intel.macosx_10_10_x86_64.whl
Collecting cmake (from -r requirements-dev.txt (line 3))
  Using cached https://pypi.uberinternal.com/packages/59/20/d1c7e0df29789e7c3aaef17d9952ed477d197cdb4e72d5e23beaf7f3e0c4/cmake-3.15.2.tar.gz
    ERROR: Command errored out with exit status 1:
     command: /Users/ajfriend/work/temp/h3-py/env/bin/python3.7 -c 'import sys, setuptools, tokenize; sys.argv[0] = '"'"'/private/var/folders/vg/wbk5fpk93z3cnpfn88yzjdbw0000gn/T/pip-install-bt50fay9/cmake/setup.py'"'"'; __file__='"'"'/private/var/folders/vg/wbk5fpk93z3cnpfn88yzjdbw0000gn/T/pip-install-bt50fay9/cmake/setup.py'"'"';f=getattr(tokenize, '"'"'open'"'"', open)(__file__);code=f.read().replace('"'"'\r\n'"'"', '"'"'\n'"'"');f.close();exec(compile(code, __file__, '"'"'exec'"'"'))' egg_info --egg-base pip-egg-info
         cwd: /private/var/folders/vg/wbk5fpk93z3cnpfn88yzjdbw0000gn/T/pip-install-bt50fay9/cmake/
    Complete output (5 lines):
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/private/var/folders/vg/wbk5fpk93z3cnpfn88yzjdbw0000gn/T/pip-install-bt50fay9/cmake/setup.py", line 7, in <module>
        from skbuild import setup
    ModuleNotFoundError: No module named 'skbuild'
    ----------------------------------------
ERROR: Command errored out with exit status 1: python setup.py egg_info Check the logs for full command output.
make: *** [init] Error 1

@ajfriend
Copy link
Collaborator

The error goes away if I remove cmake from the requirements-dev.txt.

But I guess this then requires the user to have previously installed cmake via something like brew install cmake. Would having it in requirements-dev.txt remove the need for this?

@ajfriend
Copy link
Collaborator

Looks great to me. Thanks so much @ankitmehta21! I'm happy to merge once tests pass.

@ajfriend ajfriend merged commit 6d8a86d into uber:cython Sep 24, 2019
@ankitmehta21 ankitmehta21 deleted the cython-ankit branch September 24, 2019 03:04
ajfriend added a commit that referenced this pull request Sep 25, 2019
* 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.
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>
@SirDagen SirDagen mentioned this pull request May 22, 2025
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