search

DerThorsten / nifty

A nifty library for graph based image segmentation.
MIT License
41 stars 21 forks source link

Cannot use uint64 in nifty.graph.rag.gridRag #105

Closed hanslovsky closed 7 years ago

hanslovsky commented 7 years ago

nifty.graph.rag.gridRag requires the labels to be np.uint32. https://github.com/DerThorsten/nifty/blob/master/src/python/module/nifty/graph/rag/__init__.py#L20 Allowing for np.uint64 (and potentially more data types) would be desirable

DerThorsten commented 7 years ago

I guess we should just make the default label type uint64. Allowing for more data types will increase the class-complexity since on the python side this would result in a new class.

hanslovsky commented 7 years ago

I could live with np.uint64 as default. I think, accepting the two biggest unsigned integer types (np.uint32 and np.uint64) would still be ok complexity-wise. All other cases could then default to np.int64.

DerThorsten commented 7 years ago

Ok, I guess allowing for uin32 and uint64 is fine. Would you also like int32 and int64 to work? ( One could argue in the python world signed integers are more common)

hanslovsky commented 7 years ago

In principle I am ok with using all integer types (in #109 it's just unsigned right now) which would be just 16 lines of code. If you'd like to exclude 8bit and 16bit integers, that's fine with me. Just let me know what you prefer and I'll adapt the pr accordingly.

DerThorsten commented 7 years ago

Ok, fuck it, just use all god damn integers...

DerThorsten commented 7 years ago

but we maybe should watch the binary size / *.so size. I guess if it is only increasing by lets say max. 0.1to 1 MB I guess one should not give a fuck

hanslovsky commented 7 years ago

I'll build and compare.

hanslovsky commented 7 years ago

I built the library eight times, adding an additional integer overload each time:

du -hs python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so

3.7M    python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so
3.8M    python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so
3.9M    python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so
4.0M    python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so
4.1M    python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so
4.3M    python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so
4.4M    python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so
4.5M    python/nifty/graph/rag/_rag.cpython-36m-x86_64-linux-gnu.so

It is save to say that adding an additional overload adds about 0.1MB to the library, so a total of about 0.8MB when considering all of {uint,int}_{8,16,32,64}.

DerThorsten commented 7 years ago

I guess this is ok, ty for the experiments