Use new H3MemoryManager system #261

ajfriend · 2022-06-05T03:17:56Z

No description provided.

codecov · 2022-06-05T03:20:27Z

Codecov Report

Merging #261 (50c649c) into dev_v4 (cfbd892) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            dev_v4      #261   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           17        17           
  Lines          425       427    +2     
=========================================
+ Hits           425       427    +2

Impacted Files	Coverage Δ
src/h3/__init__.py	`100.00% <ø> (ø)`
src/h3/api/_api_template.py	`100.00% <ø> (ø)`
src/h3/_cy/__init__.py	`100.00% <100.00%> (ø)`
src/h3/api/basic_int/_binding.py	`100.00% <100.00%> (ø)`
src/h3/api/basic_str/_binding.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfbd892...50c649c. Read the comment docs.

`polyfill_polygon`. some memory error. not sure what's happening just yet.

isaacbrodsky · 2022-06-05T22:13:05Z

src/h3/_cy/memory.pyx

+ return (<H3int[:]>a)[:0]
+
+
+cdef class H3MemoryManager:


I'd expect a memory manager to have broader responsibilities for managing allocations (have more verbs essentially); this seems to be more like a wrapped buffer or a block of managed memory?

Yeah, basically just an object to be responsible for deallocating the memory before that responsibility is handed off to a memoryview object.

Open to different names :)

Can we make this a context manager? Seems like a good fit unless for some reason a context manager is hard/slow in Cython

I was also thinking about using a context manager here. I forget the specific examples, but it seemed like not every situation naturally fell into a context manager pattern.

An object that guarantees memory clean up would, at least, be an improvement over what we have now. I wouldn't be opposed to refactoring that into a context manager in the future.

Either way, I'll keep it in mind as I play with this. And if a context manager does end up working out, maybe I'll do that.

kylebarron · 2022-06-06T19:41:08Z

src/h3/_cy/memory.pyx

+ return i
+
+
+cdef H3int[:] empty_memory_view():


I know this is just copying an existing function, but can this have a docstring of what this function is used for? A memoryview isn't growable iirc, so not clear to me what the point of an empty memory view is.

Ah, ok. I can try to add one.

The point of an empty memory view is that we could have situations where functions return an empty set.

IIRC, this was the cleanest way to do that. The syntax for creating one was pretty ugly otherwise, as far as I could figure it out, so I banished it to this function.

It'd be amazing if someone figured out a cleaner way to do this!

kylebarron · 2022-06-06T19:56:12Z

src/h3/_cy/memory.pyx

+ return (<H3int[:]>a)[:0]
+
+
+cdef class H3MemoryManager:


Can we make this a context manager? Seems like a good fit unless for some reason a context manager is hard/slow in Cython

* remove scripts/examples/apps * BUILD_FUZZERS OFF * ENABLE_FORMAT off * turn off all the things (and sort options) * mac attack * see if there's a build time difference * back to the future

isaacbrodsky · 2022-07-25T22:08:14Z

src/h3/_cy/cells.pyx

- faces = {f for f in faces if f >= 0}
- stdlib.free(ptr)
+ # todo: wait? do faces start from 0 or 1?
+ # we could do this check/processing in the int_mv object


This doesn't seem like something you would want in the int_mv object itself, as it's very application specific

isaacbrodsky · 2022-07-25T22:08:37Z

src/h3/_cy/geo.pyx

@@ -63,6 +71,7 @@ cdef h3lib.GeoLoop make_geoloop(geos, bool lnglat_order=False) except *:

 gl.numVerts = len(geos)

+ # todo: need for memory management


I'm not sure what this means.

isaacbrodsky · 2022-07-25T22:09:09Z

src/h3/_cy/cells.pyx

+ # todo: mystery: why does this break in the tests when you move it before the error check?
+ # ideally, the create_mv method would be robust to borked data...
+ mv = hmm.create_mv()


What exactly breaks? Does this cause an exception to get swallowed?

isaacbrodsky · 2022-07-25T22:10:24Z

src/h3/_cy/h3lib.pxd

@@ -10,7 +9,7 @@ cdef extern from "h3api.h":
 cdef int H3_VERSION_MINOR
 cdef int H3_VERSION_PATCH

- ctypedef uint64_t H3Index
+ ctypedef uint64_t H3int "H3Index"


Why is this getting renamed?

So that the Cython code only has one name for this concept. I'm using H3int here since the Python library deals with both integer and string representations of an H3Index, so and the names H3int and H3str help make it clear which one it is.

isaacbrodsky · 2022-07-25T23:21:57Z

src/h3/_cy/memory.pxd

+todo: should i be allocating with python memory functions?
+https://cython.readthedocs.io/en/latest/src/tutorial/memory_allocation.html


I would expect this is preferred

It turns out there are some interesting trade-offs we might consider around acquiring the GIL and Python/Cython versions. Checkout the changes here: bf5fb8a

cc: @kylebarron

isaacbrodsky · 2022-07-25T23:26:46Z

src/h3/_cy/memory.pyx

+ self.n = n
+ self.ptr = <H3int*> stdlib.calloc(self.n, sizeof(H3int))
+
+ if (self.n > 0) and (not self.ptr):


nit: are the parenthesis needed?

isaacbrodsky · 2022-07-25T23:27:50Z

src/h3/_cy/memory.pyx

+ return (<H3int[:]>a)[:0]
+
+
+cdef class H3MemoryManager:


Please add a docstring as to what this class does. It's not immediately obvious it's specific to H3 indexes, although I think that's the case.

isaacbrodsky · 2022-07-25T23:29:07Z

src/h3/_cy/memory.pyx

+ # should be NULL, and deallocing on NULL is fine.
+ # If the pointer is *not* NULL, then this means the MemoryManager
+ # has is still responsible for the memory (it hasn't given the memory away to another object).
+ stdlib.free(self.ptr)


Set self.ptr to NULL?

…ure there's a way to make them private

Co-authored-by: AJ Friend <[email protected]>

ajfriend · 2022-08-04T22:51:08Z

Got history got weird. Moving these changes over to #268

* Squash #261: Use new H3MemoryManager system Co-authored-by: AJ Friend <[email protected]> * add some error checks * comments * comments * clearer logical flow * arrays are different from memoryviews! * little cleaner * Revert "comments" This reverts commit 80c3f4d. * Revert "Revert "comments"" This reverts commit a9d6590. * Revert "comments" This reverts commit 24904bd. * comments * arrays are different from memoryviews! * try _copy_to_mv * i don't get enough opportunities to consider using the word "penultimate" * ugh, and there i go using it incorrectly... * hmmm. was this really all it was? * That's what it was! This reverts commit 7e72adb. * clarify branches * remove simple_mv * moving around * clean up * comments * comments * comments Co-authored-by: Isaac Brodsky <[email protected]>

* WIP: Starting work on v4, The Sequel (#238) * cimport for h3fake2 doesn't seem to work * back to where we were * meh, close to what i was hoping for * h3fake2 in github actions * comment out current bindings that consume the h3c v3 library * don't need the coveragerc file at the moment; clear out c files in /tests * sdist needs h3fake2 * try CIBW_BEFORE_BUILD to get h3fake2 * need h3fake2 for cibuildwheel tests * update h3 C library * comment out most of h3lib.pxd * calling from v4 c lib works! (tests dont' work yet tho) * tests shall pass * starting to move over first few functions * little steps * util submodule fully moved over; some hackery with h3fake2 errors * getting even trickier with h3fake2 errors * few more simple functions * geo_to_h3 and h3_to_geo * grid_distance function * grid_disk * parents and kids * compact/uncompact * getNumCells * comment out sdist test temporarily * commenting out `make_sdist` seems to have turned off the other CI jobs * no, it was that we're pushing to the dev branch * keep the sdist job in there, but commented out * ajfriend/more v4 funcs (#241) * get_pentagon_indexes and get_res0_indexes * line * cell area functions * get_faces (#248) * initial pass * thoughts * the results of thinking * this commit is associated with the previous commits * nogil * i'm a bill * remove the typical nonsense * v4: local ij functions to v4 cython (#240) * move local ij functions to v4 implementation * revert some changes and add gridPathCells * swap implementation * revert gridPathCells * ring (#251) * Uncomment cython example, and exclude sdist from wheels test temporarilty (#252) * uncomment cython test * exclude make_sdist temporarily from the wheels tests * v4: polygonToCells (#254) * v4: geo boundary, set to multi polygon (#255) * v4: cell boundary * v4: h3_set_to_multi_polygon * v4: convert edge from h3fake2 (#253) * v4: convert edge from h3fake2 * on error not neighbors * except * * check for neighbor error * Fix sdist build (#239) * make sure `make_sdist` CI job still broken * reproduce locally * update h3lib * still seems broken * try sdist on mac and ubuntu * pyest! * debugging * ensurepath * Disable building docs * just build sdist once * colon Co-authored-by: Isaac Brodsky <[email protected]> * remove h3fake2 transitional library (#257) * Remove more sdist files (#259) * remove scripts/examples/apps * BUILD_FUZZERS OFF * ENABLE_FORMAT off * turn off all the things (and sort options) * mac attack * see if there's a build time difference * back to the future * New error system (#260) * starting to play around with new error system * move enum to h3lib.pxd * this structure is weird :( * error system gets its own file * go back to using H3ErrorCodes * extern the H3ErrorCodes * remove H3Index in favor of just H3int * " to ' * import ordering * something * exception hierarchy * formatting * errors list * remove H3ResolutionError * remove H3EdgeError * remove H3CellError * remove H3DistanceError * H3FailedError * domain error * H3UnrecognizedException * most of em done * remove H3ValueError * remove some checks * ideas ideas * document what the function raises? * thoughts * lint * one half idea for DRYing (kinda) * potentially make the E_SUCCESS path faster * typos * maple syrup * cimport H3ErrorCodes as ec * clean up * whitespace * clarify cases * trying new error idiom * thanks comment * notes * applied idiom a few times now * alternative idiom? * handling some more potential errors * idioms and ideas * clean * removing check * interesting allocation example * lint * just trying to beat 2.7 for sport right now * works? * bow to lint * using enum values directly! * now that's a hierarchy * remove raise_with_msg * ij funcs * lint * remove some checks * remove err * comment * cleaned up `ring`, and i think i fixed a memory leak bug * think i fixed another memory leak bug * another memory leak bug * memory leaks! * clean * removing some comments * meh * H3Exception shouldn't be appearing as a concrete object * renaming some base exceptions * clean up * don't have a test for this anyway * documented * puttering around * words * backing up this nonsense with an airtight argument * air toight * todos * more words * skepticism * rewording * editing * ordering * tests on the error codes * more error code tests * note * address some comments * we raise! * lint * comments * Squash #261: Use new H3MemoryManager system (#268) * Squash #261: Use new H3MemoryManager system Co-authored-by: AJ Friend <[email protected]> * add some error checks * comments * comments * clearer logical flow * arrays are different from memoryviews! * little cleaner * Revert "comments" This reverts commit 80c3f4d. * Revert "Revert "comments"" This reverts commit a9d6590. * Revert "comments" This reverts commit 24904bd. * comments * arrays are different from memoryviews! * try _copy_to_mv * i don't get enough opportunities to consider using the word "penultimate" * ugh, and there i go using it incorrectly... * hmmm. was this really all it was? * That's what it was! This reverts commit 7e72adb. * clarify branches * remove simple_mv * moving around * clean up * comments * comments * comments Co-authored-by: Isaac Brodsky <[email protected]> * remove: from h3 import h3 (#270) * is_valid_cell (#269) * Remove alias functions (#271) * remove alias function hex_range * note test_k_ring and test_hex_range are the same * remove test_hex_range * duplicate tests * remove test_hex_range2 * same test * remove test_hex_range_pentagon * remove alias: k_ring_distances * tests are the same! * same test * combine tests * remove alias: h3_is_res_class_iii * lint * Update most functions to v4.0 names (#272) * grid_distance * cell_to_parent * get_resolution * is_pentagon * is_res_class_III * get_pentagons * get_res0_cells * get_base_cell_number * compact and uncompact * get_faces * cell_to_boundary * cell_to_latlng * latlng_to_cell * cell_to_children * int_to_string and string_to_int * cell_to_center_child * moving things around * is_valid_directed_edge * grid_path_cells * moving around * cells_to_multi_polygon * are_neighbor_cells * cell_to_local_ij and local_ij_to_cell * get_icosahedron_faces * great_circle_distance * get_num_cells * note * remove hex_range_distances and hex_ranges (easily implementable by user) * grid_ring * grid_disk * moving around * origin_to_directed_edges * directed_edge_to_boundary * cells_to_directed_edge * directed_edge_to_cells * todo: directedEdgeToCells * get_directed_edge_origin * get_directed_edge_destination * grouping functions * more grouping * _binding to _b * todos * average_hexagon_area * average_edge_length * average_edge_length and edge_length * average_hexagon_edge_length * ordering * more ordering * Get the makefile working on Linux (#274) * bump core lib to 4.0 rc5 (#275) * bump core lib to 4.0 rc5 * more pythonic names for string<->int conversion functions * cython hex2int to str_to_int to match the python functions * better argument names for great_circle_distance * great_circle_distance comments * revert great_circle_distance tuple unpacking since it breaks python2. drop python 2 before release * polygon_to_cells (#276) * start converting tests over * converted main tests * linting * better null island test * test_compact_and_uncompact_cells * convert a few more tests * add h3.Polygon class * lint * migrating some tests * this shift_circular_list thing makes these tests too hard to understand... * remove shift_circular_list and convert a few more tests * last of the tests in test_h3.py converted * last of the tests converted * note for polygons_to_cells in the future * Try out a __repr__ for h3.Polygon * test_polygon_class.py * use `cells` instead of `hexes` throughout the library and tests * docstring for Polygon * docstrings for functions * use `res` convention for resolution parameters Co-authored-by: Isaac Brodsky <[email protected]> Co-authored-by: David Ellis <[email protected]>

ajfriend added 2 commits June 4, 2022 20:10

initial commit

572a09b

hmmmmmmm

34b7eed

ajfriend added 20 commits June 4, 2022 20:27

H3Index -> H3int

0daa79b

hmm

ffb8c8a

int_mv rides into town

6f62aab

the mystery of the int[:] distances

f8a6505

ring and children

c38be89

mysteries abound!

b0b0472

thinking thoughts

fd8a9c2

so, check this out: this works

b74f7ef

and this one gives an error. the game is afoot.

5e02924

good ol' fashioned detective work will catch the crook

f70f116

just good taste, really

b787da9

line

5829730

oh boy. didn't actually break on this one.

f4471d3

but back to what's good, just to be safe

0354013

these work

2ceaa5c

get_faces

f2fd2b7

cells.pyx is moved over

9669632

i think that covers edges.pyx

f918c91

so i can get this far, but i run into issues when i try to convert

cec9c84

`polyfill_polygon`. some memory error. not sure what's happening just yet.

even with the error checking...

12b6370

ajfriend mentioned this pull request Jun 5, 2022

New error system #260

Merged

isaacbrodsky reviewed Jun 5, 2022

View reviewed changes

notes

03a27e7

kylebarron reviewed Jun 6, 2022

View reviewed changes

Remove more sdist files (#259)

95c9aff

* remove scripts/examples/apps * BUILD_FUZZERS OFF * ENABLE_FORMAT off * turn off all the things (and sort options) * mac attack * see if there's a build time difference * back to the future

isaacbrodsky approved these changes Jul 2, 2022

View reviewed changes

isaacbrodsky approved these changes Jul 25, 2022

View reviewed changes

ajfriend added 25 commits July 30, 2022 17:13

Merge branch 'ajfriend/memory_system' into dev_v4_2

d626e67

Not sure why, but self.ptr = NULL fixes the bug

d126583

remove create_ptr and create_mv

24eef34

meh

85ab47d

bah

4d90557

works!

b2307b0

clean up

e945f1f

works

e8b2457

clean

8d2acb9

maybe we don't need this?

7307e97

comments

28dc369

line

7afc536

nice and clean

55c4343

mv_from_iter should be under memory module

7811697

move around

8a88133

rename to iter_to_mv

11aa2eb

memory allocator options

bf5fb8a

_remove_zeros helper method

b8f53db

little cleanup

e1f56db

add method: _create_mv

bfa13ed

don't need the n==0 case

49e1147

another comment on n==0

865326a

mv is better than x

2d8f416

organize

3b3f927

avoiding superfluous methods in the memory manager, because I'm not s…

50c649c

…ure there's a way to make them private

isaacbrodsky pushed a commit that referenced this pull request Aug 4, 2022

Squash #261: Use new H3MemoryManager system

1a87fa4

Co-authored-by: AJ Friend <[email protected]>

ajfriend closed this Aug 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new H3MemoryManager system #261

Use new H3MemoryManager system #261

ajfriend commented Jun 5, 2022

codecov bot commented Jun 5, 2022 •

edited

Loading

isaacbrodsky Jun 5, 2022

ajfriend Jun 6, 2022

kylebarron Jun 6, 2022

ajfriend Jul 3, 2022

kylebarron Jun 6, 2022

ajfriend Jul 3, 2022

kylebarron Jun 6, 2022

isaacbrodsky Jul 25, 2022

isaacbrodsky Jul 25, 2022

isaacbrodsky Jul 25, 2022

isaacbrodsky Jul 25, 2022

ajfriend Jul 31, 2022

isaacbrodsky Jul 25, 2022

ajfriend Jul 31, 2022 •

edited

Loading

ajfriend Jul 31, 2022

isaacbrodsky Jul 25, 2022

isaacbrodsky Jul 25, 2022

isaacbrodsky Jul 25, 2022

ajfriend commented Aug 4, 2022

		@@ -63,6 +71,7 @@ cdef h3lib.GeoLoop make_geoloop(geos, bool lnglat_order=False) except *:

		gl.numVerts = len(geos)

		# todo: need for memory management

		todo: should i be allocating with python memory functions?
		https://cython.readthedocs.io/en/latest/src/tutorial/memory_allocation.html

Use new H3MemoryManager system #261

Use new H3MemoryManager system #261

Conversation

ajfriend commented Jun 5, 2022

codecov bot commented Jun 5, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajfriend Jul 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajfriend commented Aug 4, 2022

codecov bot commented Jun 5, 2022 •

edited

Loading

ajfriend Jul 31, 2022 •

edited

Loading