pylinac
pylinac copied to clipboard
jrkerns
Use vectorized_filter for gamma_2d
Partially addresses https://github.com/jrkerns/pylinac/issues/562
This uses the prerelease of scipy 1.16 to try out vectorized_filter for gamma_2d.
The improvement in speed for a 400x400 image is from ~7 seconds to 0.008 seconds(so ~1000x improvement). I think some of the improvement could come if we just used generic_filter, so I can switch to using that if that works better, but the difference won't be nearly as drastic(though I expect it'll still be pretty drastic).
One problem is needing to bump the required python version to 3.11 from 3.9. I'll leave this in draft mode until scipy 1.16 is released, which should be mid-june per https://discuss.scientific-python.org/t/proposed-release-schedule-for-scipy-1-16-0/1883
Marked as ready for review. Let me know if you'd like a similar conversation for other gamma functions @jrkerns and I'll be happy to work on them!
@jrkerns looks like some merge conflicts(some of your own performance improvements it looks like!). Let me know if I should resolve them.
(I'm also kinda getting the itch to just put vectorized_filter on everything here so I may just do that)
Hi @purepani my apologies for not replying. 1000x is quite the jump! I appreciate the effort. Looping in @joaomsilveira here as he has joined the Radformation team mostly working on pylinac and made the improvements on his own.
I realize I didn't make this clearer in the documentation, but pylinac supports whatever Python version is still being supported (see the v3.23 release note; definitely need to make that clearer in the guides!) Here's the end of life graph: https://endoflife.date/python. This means we need to support python 3.9 for a few more months, then we can set the minimum to 3.10. If you want to make a switch statement where we use your vectorized filter if we are running python 3.11 + scipy 1.16 then that's fine with me. Let's see what @joaomsilveira thinks as well.
Impressive stuff. Sorry again for the late reply.
Oh no problem; I just try and ping maintainers when I see merge conflicts. I think having 2 versions of the same function isn't a great idea(which it would have to be since the implementations are completely different, and any bugs wouldn't be matched up between implementations. Especially when doing this throughout the code base, you'd essentially double the maintenance burden of each function, particularly because the way it's implemented is quite different.
The alternative is to vendor the vectorized_filter implementation from scipy until you are able to support the minimum python version. I suppose this won't work though since this project is MIT licensed and scipy is BSD licensed. There's probably some way around that issue(like having a separate package or something that is BSD licensed that you have as a dependency), but that is probably too much effort for this solution.
The alternate alternative is to bump up the python version, though that is a pretty drastic option. That said, maybe it's a good option to follow https://scientific-python.org/specs/spec-0000/ as a guideline instead of supporting all python versions until EOL, assuming you have no particular reason to support the older python versions(which is probably an incorrect assumption). Since this is a pretty drastic option, this is probably not a real solution here, but I thought I'd at least link that SPEC.
Other than that, I can probably manually modify the code to do what vectorized_filter is doing with some effort, and then you can switch to vectorized_filter once your support window reaches it. That's probably the most reasonable solution, though it'd certainly be nice to just use vectorized_filter.
Went ahead and rebased since the rebase was just replacing the new implementation.