ENH: Integrate FastBilateralImageFilter from remote module #5134

dzenanz · 2025-01-16T16:26:22Z

"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

No API changes were made (or the changes have been approved)
No major design changes were made (or the changes have been approved)
Added test (or behavior not changed)
Updated API documentation (or API not changed)
Added license to new files (if any)
Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5

"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.

jhlegarreta · 2025-01-17T13:39:47Z

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.

dzenanz · 2025-01-17T14:22:53Z

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.

N-Dekker · 2025-01-17T15:54:02Z

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) InsightSoftwareConsortium/ITKFastBilateral@451baba

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.

dzenanz · 2025-01-17T16:10:10Z

Just in October I wrote this code:

if (smoothingSigma > 0.5)
{
  // use FastBilateralImageFilter
  ...
}
else
{
  // use BilateralImageFilter
  ...
}

So yes, it is totally relevant.

N-Dekker · 2025-01-17T16:44:38Z

Modules/Filtering/ImageFeature/include/itkFastBilateralImageFilter.h

+
+template <class TInputImage, class TOutputImage>


ITK started using typename for template parameters back in 2013, especially commit 86bbb9d by Hans (@hjmjohnson)

N-Dekker · 2025-01-17T16:48:23Z

Modules/Filtering/ImageFeature/include/itkFastBilateralImageFilter.h

+  double               m_RangeSigma;
+  DomainSigmaArrayType m_DomainSigma;


Nowadays we would use data member initializers:

double m_RangeSigma{ 50.0 }; DomainSigmaArrayType m_DomainSigma{ DomainSigmaArrayType::Filled(40.0) };

N-Dekker · 2025-01-17T16:49:37Z

Modules/Filtering/ImageFeature/include/itkFastBilateralImageFilter.h

+  FastBilateralImageFilter()
+  {
+    m_DomainSigma.Fill(4.0);
+    m_RangeSigma = 50.0;
+  }


Nowadays we would do = default, and use default member initializers.

N-Dekker · 2025-01-17T16:50:21Z

Modules/Filtering/ImageFeature/include/itkFastBilateralImageFilter.h

+    m_RangeSigma = 50.0;
+  }
+
+  virtual ~FastBilateralImageFilter() {}


Nowadays:

~FastBilateralImageFilter() override = default;

N-Dekker · 2025-01-17T16:53:39Z

Modules/Filtering/ImageFeature/test/CMakeLists.txt

+    itkFastBilateralImageFilterTest.cxx
+    itkFastBilateralImageFilterTest2.cxx
+    itkFastBilateralImageFilterTest3.cxx


I'm not a fan of test names like "Test2", "Test3", etc. Hope we can eventually move towards more GTest style test, and use more meaningful names for tests.

N-Dekker · 2025-01-17T16:58:16Z

Modules/Filtering/ImageFeature/test/itkFastBilateralImageFilterTest3.cxx

+  // Generate test image
+  itk::ImageFileWriter<ImageType>::Pointer writer;
+  writer = itk::ImageFileWriter<ImageType>::New();
+  writer->SetInput(filter3->GetOutput());
+  writer->SetFileName(av[2]);
+  writer->Update();


Nowadays just one line of code:

itk::WriteImage(filter3->GetOutput(), av[2]);

N-Dekker · 2025-01-17T16:59:03Z

Modules/Filtering/ImageFeature/test/itkFastBilateralImageFilterTest2.cxx

+  // Generate test image
+  itk::ImageFileWriter<ImageType>::Pointer writer;
+  writer = itk::ImageFileWriter<ImageType>::New();
+  writer->SetInput(filter->GetOutput());
+  writer->SetFileName(av[2]);
+  writer->Update();


Nowadays just one line of code:

itk::WriteImage(filter3->GetOutput(), av[2]);

N-Dekker · 2025-01-17T17:01:56Z

Modules/Filtering/ImageFeature/test/itkFastBilateralImageFilterTest.cxx

+  using WriterType = itk::ImageFileWriter<ImageType>;
+  WriterType::Pointer writer = WriterType::New();
+  writer->SetFileName(outputImageFileName);
+  writer->SetInput(filter->GetOutput());
+  writer->SetUseCompression(true);
+
+  ITK_TRY_EXPECT_NO_EXCEPTION(writer->Update());


Nowadays just one line of code:

itk::WriteImage(filter->GetOutput(), outputImageFileName, true);

Is `ITK_TRY_EXPECT_NO_EXCEPTION still necessary? With GTest, a unit test that throw an exception (uncaught) is automatically reported as test failure.

dzenanz · 2025-01-17T17:03:58Z

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).

N-Dekker · 2025-01-17T17:08:05Z

Modules/Filtering/ImageFeature/include/itkFastBilateralImageFilter.hxx

+  gridImage->Allocate();
+  // Init values of image to 0
+  gridImage->FillBuffer(0.0);


Nowadays, no need to call Allocate(); FillBuffer(0.0), just call:

gridImage->AllocateInitialized();

N-Dekker · 2025-01-17T17:11:34Z

Modules/Filtering/ImageFeature/include/itkFastBilateralImageFilter.hxx

+    for (size_t i = 0; i < ImageDimension + 1; ++i)
+    {
+      gridStartPos[i] = 0;
+    }
+
+    GridRegionType region;
+    region.SetSize(gridSize);
+    region.SetIndex(gridStartPos);
+    gridImage->SetRegions(region);


I would remove both local variables gridStartPos and region. gridStartPos just contains zero's. The region size of the gridImage can be specified directly, without local region variable, by:

gridImage->SetRegions(gridSize);

N-Dekker · 2025-01-17T17:18:36Z

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.

blowekamp · 2025-01-17T17:24:32Z

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?

dzenanz · 2025-01-17T17:27:44Z

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.

jhlegarreta · 2025-01-18T14:48:12Z

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:
thewtex@9e9d513

dzenanz added 2 commits January 16, 2025 09:57

ENH: Integrate FastBilateralImageFilter from remote module

721b6f2

"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.

DOC: Improve documentation of FastBilateralImageFilter

53b8ba3

dzenanz added 2 commits January 16, 2025 12:22

ENH: Remove remote module option and add migration guide entry

3a97949

DOC: Fix spelling

039b2bd

github-actions bot added area:Remotes Issues affecting the Remote module area:Documentation Issues affecting the Documentation module labels Jan 16, 2025

N-Dekker reviewed Jan 17, 2025

View reviewed changes

jhlegarreta mentioned this pull request Jan 23, 2025

Improve ApplyScriptToRemotes.sh script's robustness #1024

Open

This was referenced Feb 2, 2025

COMP: Bump to minimum CMake 3.16.3 InsightSoftwareConsortium/ITKLabelErodeDilate#41

Merged

ENH: Transition package build test GHA to modern remote infrastructure InsightSoftwareConsortium/ITKThinShellDemons#24

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Integrate FastBilateralImageFilter from remote module #5134

ENH: Integrate FastBilateralImageFilter from remote module #5134

dzenanz commented Jan 16, 2025

jhlegarreta commented Jan 17, 2025

dzenanz commented Jan 17, 2025

N-Dekker commented Jan 17, 2025

dzenanz commented Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker Jan 17, 2025

dzenanz commented Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker Jan 17, 2025

N-Dekker commented Jan 17, 2025

blowekamp commented Jan 17, 2025

dzenanz commented Jan 17, 2025

jhlegarreta commented Jan 18, 2025 •

edited

Loading

ENH: Integrate FastBilateralImageFilter from remote module #5134

Are you sure you want to change the base?

ENH: Integrate FastBilateralImageFilter from remote module #5134

Conversation

dzenanz commented Jan 16, 2025

PR Checklist

jhlegarreta commented Jan 17, 2025

dzenanz commented Jan 17, 2025

N-Dekker commented Jan 17, 2025

dzenanz commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzenanz commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Dekker commented Jan 17, 2025

blowekamp commented Jan 17, 2025

dzenanz commented Jan 17, 2025

jhlegarreta commented Jan 18, 2025 • edited Loading

jhlegarreta commented Jan 18, 2025 •

edited

Loading