ENH: Integrate FastBilateralImageFilter from remote module
"New" files are taken directly from https://github.com/InsightSoftwareConsortium/ITKFastBilateral at commit e37be230b50c6934b0c260c2ffec8092bf54f498.
Modified files are merger of corresponding content from the remote module.
This is the first remote module to be integrated as part of #3371.
PR Checklist
- [x] No API changes were made (or the changes have been approved)
- [x] No major design changes were made (or the changes have been approved)
- [x] Added test (or behavior not changed)
- [x] Updated API documentation (or API not changed)
- [x] Added license to new files (if any)
- [x] Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
Unless all style and compiler changes that have been contributed in the past 2 to 3 months by Hans and Niels are applied to this code, we will be introducing code having a discouraged style.
There are multiple remote modules in the same boat. Should we integrate them, and then make code style changes pass on the entire source, including the newly added code? Or require those style changes be made before integrating? The second option will probably require more effort. @N-Dekker and @hjmjohnson might want to pitch in.
Is the FastBilateralImageFilter still relevant nowadays? I see, there was a publication of the filter by Jordan Woehr on 2009-08-28, A Fast Approximation to the Bilateral Filter for ITK. The remote module was originally by Pablo (@phcerdan) https://github.com/InsightSoftwareConsortium/ITKFastBilateral/commit/451baba6d77f85c0a4ffccfbbde1adc0acd9721b
In general I would suggest people to double-check if a remote module is still relevant, before integrating them into the core ITK Modules. Otherwise ITK will just grow and grow, increasing the maintenance burden and increasing build times.
Just in October I wrote this code:
if (smoothingSigma > 0.5)
{
// use FastBilateralImageFilter
...
}
else
{
// use BilateralImageFilter
...
}
So yes, it is totally relevant.
Niels, it would be good if you tackled these (and possibly other changes) yourself. Push changes either to my branch from which this PR is open, or in the current remote module (I would pick up the changes from there later and integrate them in this PR).
Niels, it would be good if you tackled these (and possibly other changes) yourself.
@dzenanz Sorry, I'm just doing a little bit of reviewing of this PR now. By the way, none of the comments I made so far look like showstoppers to me. They could also be addressed after merging. Or you could directly address them yourself.
Could we consider an alternative name to "FastBilateralImageFilter". Anything more descriptive of the name? If another version is created that is even faster, how would it be distinguished?
I would prefer not to rename it during transition. Having the same class name will make it easier for people to find it. We could rename it in the future, either with next major version of ITK (e.g. v7), or when another implementation appears.
There are multiple remote modules in the same boat. Should we integrate them, and then make code style changes pass on the entire source, including the newly added code? Or require those style changes be made before integrating? The second option will probably require more effort. @N-Dekker and @hjmjohnson might want to pitch in.
IMO, all such changes should be applied to the remotes prior to integrating them, even when integrating them is not yet on the roadmap. We tend to forget about examples, modules not turned on by default and remotes. The later may be more difficult, but there have been multiple efforts over time to try to apply sweeping changes trying to avoid the burden it implies doing it one by one/separately/manually on each local clone, e.g.
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Utilities/Maintenance/ApplyScriptToRemotes.sh
From issue #4748: "Also, there is this tool Matt has used to update the remote module CI config to ITK 5.4: https://github.com/asottile/all-repos
Ongoing work: https://github.com/thewtex/ITK/commit/9e9d5131dcde8f46505c838218df68ce24701cd0