Cython next steps about h3-py HOT 7 CLOSED

We currently use git submodule to include the H3 C code. I wonder if subtree would be simpler: https://github.github.com/training-kit/downloads/submodule-vs-subtree-cheat-sheet/

from h3-py.

Also:

• Is _api_template.py the best way to generate the 3 different APIs? I'm worried that the globals().update(...) is too hacky...
• There's a common memory management pattern currently implemented with make_ptr() and make_mv() in util.pyx. We calloc memory, give it to an H3 function, then remove zero entries, realloc, and wrap it in a memoryview. Rather than two functions, would this be better done with a context manager? Or some other idiom, like passing the H3 function in as a lambda to a single memory management function?
• Make numpy an optional dependency for package install

from h3-py.

Also, add the fallback logic for the slower path in hexRing in the case that the fast path runs into a pentagon.

from h3-py.

We currently use git submodule to include the H3 C code. I wonder if subtree would be simpler: https://github.github.com/training-kit/downloads/submodule-vs-subtree-cheat-sheet/

I'm not a fan of submodules -- it's easy to mess things up, and I often have to wipe everything out and start over to get my submodule working. If subtrees are better / cleaner (I've not used them yet) then I'm all in favor.

Can we use a src directory structure to make sure that tests run only from the installed package? (Like here). I've tripped over this a few times when coming back to development after a while.

You mean h3-py/src/h3/? I'm in favor. Will require some changes to cmake scripts, etc. Will make things cleaner for testing.

Install package as h3 instead of h3py. @kwmsmith, can you help here? I can give it a shot, but I feel like you have more expertise.

This may be tricky, and might require that we change the name of the h3 C-lib so there isn't a name collision with cmake; if that doesn't work, we might have to get creative. I don't claim any particular cmake expertise :) It's the combination of cmake + h3 submodule + scikit-build that makes this challenging. I'm at a conference this week; won't be able to get to this until the weekend.

@kwmsmith: What else do we need to do to ensure clean and reliable builds?

Get CI working (again?) I don't know what was in place before, but if someone could get Travis / appveyor working that would be great. I do not have the bandwidth to do so myself, however.

Testing with hypothesis

💯 Getting basic hypothesis tests in place would be good, but IMO we don't need to block on this.

Memory safety testing? what's the best way to do this? valgrind?

One option would be to set up some big tests (polyfill some continent-scale polygons, for example) and mark them with a pytest mark like @pytest.mark.memory to allow easy test selection from cmdline. See here for examples:

https://doc.pytest.org/en/latest/example/markers.html

We've done this sort of thing on other projects and it's worked well.

from h3-py.

You mean h3-py/src/h3/? I'm in favor. Will require some changes to cmake scripts, etc. Will make things cleaner for testing.

Yeah. I was thinking of something like this:

h3-py
├── readme.md
├── setup.py
├── src
│   ├── h3
│   │   ├── *.py
│   │   └── *.pyx
│   └── h3lib
│       └── *.c
└── tests
    └── *.py

But actually, it probably doesn't matter what the folders under src are called, as long as the package installs as h3.

This may be tricky, and might require that we change the name of the h3 C-lib so there isn't a name collision with cmake; if that doesn't work, we might have to get creative. I don't claim any particular cmake expertise :) It's the combination of cmake + h3 submodule + scikit-build that makes this challenging. I'm at a conference this week; won't be able to get to this until the weekend.

Sounds good! Is the issue that when we clone in the submodule, the folder name defaults to h3? I wonder if sub tree/module has an option to rename.

Get CI working (again?) I don't know what was in place before, but if someone could get Travis / appveyor working that would be great. I do not have the bandwidth to do so myself, however.

Created #58

💯 Getting basic hypothesis tests in place would be good, but IMO we don't need to block on this.

Agreed that we don't need to block on this.

from h3-py.

@kwmsmith do you think you'll have a chance to take a look at this? As always, I really appreciate the help, and not sure what I'd do without it :)

from h3-py.

Closing this as we've broken the remaining points out into separate issues.

from h3-py.