32 bit dtypes => 64 bit in morphology.reconstruction#6295
32 bit dtypes => 64 bit in morphology.reconstruction#6295JamesSample wants to merge 1 commit intoscikit-image:mainfrom
morphology.reconstruction#6295Conversation
|
Thanks, @JamesSample. I will take a look soon and can help out a bit with |
|
Hi @JamesSample, I am going to be taking a look at this today. One option is to used Cython's fused types to allow both int32 and int64 variants so we don't always have to have the memory overhead of int64 when it isn't needed. One issue with the fused type approach is that we would also need a second fused type for the unsigned integers as well. Given those two fused types, Cython would generate 4 variants of the function. The following are the ones of interest:
but two unneeded variants would also be generated: (I am not aware of a way to avoid that)
Another recently supported option is to use Pythran instead. As of skimage 0.19 we do support its use in scikit-image. I think this may be a good candidate for that and will allow us to just create the variants we actually want. I will try it out today and report back |
|
This is included within the update in #6342, so let's go ahead and close this one |
Apologies for the delay. Everyone in my household came down with Covid last week, so I've mostly been at home entertaining bored kids.
This is a very limited pull request, but I likely won't get chance to look at this again until I've cleared a backlog of other stuff, and I thought it better to submit something now to keep things moving...
Description
This PR refers to #6277.
I've converted
int32dtypes toint64dtypes in both the Cython and Python code. I've tested that this version gives the same result as the original for a selection of small arrays, and it completes without error (and produces seemingly sensible results) for two arrays that were previously crashing the kernel.For indexing into arrays in Cython I've used
Py_ssize_tfor signed data types andsize_tfor unsigned, but I don't have enough experience of Cython to know whether this is the best choice.Note: This code still relies on
filters._rank_order.rank_order, which converts ranks touint32. This code will therefore still fail if2 * seed.size > np.iinfo(np.uint32).max(i.e. if the number of values in one of the input arrays is greater than about 2.1e9). This limitation could be removed by also switchingrank_orderto 64 bit dtypes. For now, this code just raises an error (instead of crashing the kernel).I'm afraid I haven't had time to make the memory handling in the Python more efficient, or to think about how to switch between 32 and 64 bit versions based on the size of the input arrays.
Checklist
./doc/examples(new features only)./benchmarks, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py.doc/release/release_dev.rst.example, to backport to v0.19.x after merging, add the following in a PR
comment:
@meeseeksdev backport to v0.19.xrun-benchmarklabel. To rerun, the labelcan be removed and then added again. The benchmark output can be checked in
the "Actions" tab.