Optimise grain finding #104
Conversation
jni
force-pushed
the
optimise-grain-finding-3
branch
from
456dc57
to
9a0664c
Compare
Using a NumPy array as a buffer increases performance, but leaves you open to bugs due to reusing the same buffer and overwriting it, which is what was happening. 😅 There's a better-performing fix still: use a buffer as big as the image, but advance the start index in the buffer as you finish each flood fill. This guarantees optimal space and time requirements (no need for copies, or a huge buffer that is mostly unused).
|
Ok, with thanks to @mikesmic for helping me debug this last week, this is mostly fixed. The issue was that I was being sooooo clever by using a NumPy array as a buffer to store the coordinates — but then when I returned the coordinates, I was returning a view into the buffer, rather than a copy — and of course the buffer was getting overwritten with each grain! 🤦 There's a better fix, which I'll try to implement next: we actually know the max number of coordinates we need: it's the size of the image. We just need to make sure we don't reuse the same buffer space. We can ensure this by copying the buffer after each fill, but we can also ensure it by making a buffer of the same size as the image, then incrementing our position along that buffer. Then the buffer will contain the coordinates of each grain, sorted by grain id, and each grain will contain a view into a section of that buffer. Then there is one more bug to address, here's cell 39 in the notebook on develop: 
And this is what it looks like on this branch: 
😅 I think the issue is probably to do with the grain centroid or extent or somesuch. I'll dig into the plotting code to check. After that, I'll try to make some proper benchmarks. |
|
Whoop whoop, it's fixed! 😊 I was just neglecting to add the seed point in one of my two accelerated flood fills, so every grain had (0, 0) in its coordinates. 😅 The notebook output is now identical in this PR and in the develop branch. 🚀 |
|
Nice! Thanks!! How much faster is it out of interest? |
|
By "feel" it's a lot. But as I mentioned I need to benchmark properly — it looks like the timings from the progress bars include a bit of time at the end (?) — the progress runs way faster but in some cases (e.g. the 6 seconds at the bottom of the timings below on my branch) it sits there at 99% for a bit. I'd say the flood fill itself is 10x-100x faster. Here's the progressbar output for reading and processing a biggish file on develop: and on this branch: You should give it a try! 😊 Actually I'll mark this as ready to review — I do want to keep chipping away at this, as I think we can get that whole pipeline down to 1-2s (though I'm not 100% about the quaternion stuff). But I think further progress should probably be done in subsequent PRs. |
Update tests to pass
|
I've added some tests of the grain finding in ebsd and hrdic for the I want to remove the flood fill methods from the Map classes, they aren't necessary now and are confusing. |
| @njit | ||
| def find_first(arr): | ||
| for i in range(len(arr)): | ||
| if arr[i]: | ||
| return i |
There was a problem hiding this comment.
LOL good Q! 🤣 I must have used it in a different setting at some point, then forgotten to delete it. 😬
Anyway it's a handy little function 🤣
defdap/ebsd.py
Outdated
| grains[y, x] = index | ||
| points_left[y, x] = False | ||
| edge = [seed] |
|
Whoo! 🥳 |
This PR is still a work in progress but it implements two improvements:
image == label(which can be expensive when done thousands of timesI'm getting weirdness with the grain IDs now so it's not ready to merge... But it's much faster! 😅