ENH: Ingest ITKFastBilateral into Modules/Filtering (supersedes #5134)#6159
Merged
hjmjohnson merged 20 commits intoInsightSoftwareConsortium:mainfrom Apr 29, 2026
Merged
Conversation
FastBilateralFilter from https://www.insight-journal.org/browse/publication/692 Exported as-is. Memory consumption is reported to be high. Reviews: ``` This filter bring great improvement in speed, with difference to classic bilateral filter being invisible to the naked eye. Evidence: They provide a test image and compare output of their filter to the output of classic filter. The choice of the image is not the greatest (differences barely visible with the image quality used). Open Science: Souce code supplied, worked out-of-the-box, test images available. Reproducibility: The code worked when I exchanged it for classic bilateral filter in my own project. Suggestion: Add a test case for 3D images. Open source Contributions: The code is very usable. Use is well described, and it took me 5 minutes to test it and use it. Code Quality : Code was readable, but I did not check it thouroughly. Free comment : Classic bilateral filter takes too long, ~4 minutes for relatively small 3D image. This filter is the solution. ``` ``` The problem of denoising images without introducing any unwanted intensity transformations is quite challenging by itself. One way of reducing the negative effects of denoising is to use local processing instead of global one. Such localization can be thought even more abstractly as processing the local vicinity of the variable. A good example of such approach is the family of bilateral filters where classification of the vicinity applies to both an intensity range and spatial domain. As a consequense, processing with bilateral filters provides very good noise removal while still preserving edges and image features at finer scales which makes the family of bilateral filters very attarctive for image processing tasks. The ITK library does have a bilateral filter implemented. However that implementation is extremely slow. The author of this submission implemented the bilateral filter that is very very fast. Evidence: The author provides the manual, source code along with input/output images thus the work could easily be validated. Open Science: The work follows the Open Science spirit. The author does provide the both source code and images. The manual is well written and is very handy for the users. Reproducibility: The reviewer was easily able to reproduce the authors' work and the outputs are idetical to those provided by the author. After downloading the code, the compilation process went without any problems and the reviewer did not experienced any problems with running the executables either. It just worked as it was supposed to. Use of Open Source Software: The author did use Open Source Software and the code was contributed as the Open Source package too. Open source Contributions: The author does provide the code in the form that allows to easy compile and use it.In the manual the author described in details the algorithm basis and provided the examples with parameters which was very useful. The process took very short time. Code Quality : The author provides the source code which is easy to read. The reviewer used this code on Linux (Debian Lenny) platform with gcc 4.2.3 and did not experience any problems. The code runs very fast. The code follows a modern coding style and the ITK rules so the reviewer would recommend this submission to be included to the ITK lib. There also should be mentioned one very usefull advantage of the submitted code: the author preserve the class API which resemble the API of analogous filter from the ITK library. The reviewer considers this as a big plus. Quality of the data : Qualit of the data is good and it is easy to use. Interest: The field of the Signal and Image denoising, where preserving edges and finer features are the most required properties. Free comment : The reviewer made some performance tests which demonstarted that the FastBilateral filter from this submission performed 200 times as much vs itk:: BilateralImageFilter. So the reviewer is very happy while using this FastBilateral filterfilter and highly recommends to include it into the ITK library. ``` Comments: @Dzenan: ``` I just now realized that for small sigmas, this filter requires immense amounts of memory! Domain sigma=1.0 with image size 448x448x15 requires 12GB of intermediate storage. ```
This includes uploading test data to my public folder in girder. https://data.kitware.com/#folder/5afde4038d777f15ebe1d6cb All tests passing
For consistency with other external modules. See for example: InsightSoftwareConsortium/ITKTotalVariation@8eb1673
Example warning: include/itkFastBilateralImageFilter.h:155:16: warning: 'GenerateInputRequestedRegion' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
…acro Added two new macro's, intended to replace the old 'itkTypeMacro' and 'itkTypeMacroNoParent'. The main aim is to be clearer about what those macro's do: add a virtual 'GetNameOfClass()' member function and override it. Unlike 'itkTypeMacro', 'itkOverrideGetNameOfClassMacro' does not have a 'superclass' parameter, as it was not used anyway. Note that originally 'itkTypeMacro' did not use its 'superclass' parameter either, looking at commit 699b66c, Will Schroeder, June 27, 2001: https://github.com/InsightSoftwareConsortium/ITK/blob/699b66cb04d410e555656828e8892107add38ccb/Code/Common/itkMacro.h#L331-L337
Use static constexpr directly now that C++11 conformance is required by all compilers. :%s/itkStaticConstMacro *( *\([^,]*\),[ \_s]*\([^,]*\),\_s*\([^)]*\)) */static constexpr \2 \1 = \3/ge 'itkStaticConstMacro(name, type, value)' became unconditionally identical to 'static constexpr type name = value' with ITK commit aec9519 "ENH: Update compiler macros (InsightSoftwareConsortium#810)", maekclena, 7 May 2019. 'itkGetStaticConstMacro(name)' became unconditionally identical to '(Self::name)' with ITK commit 84e490b "Removing some outdated compiler conditionals", Hans Johnson, 31 July 2010. Most 'itkStaticConstMacro' calls were removed by ITK commit 5c14741
The SetUseImageSpacingOff function was deprecated
The code already uses C++ 'using' declarations. Update the corresponding comments to say 'type aliases' instead of the legacy 'typedefs' terminology.
Brings FastBilateral from a configure-time remote fetch into the ITK source tree at Modules/Filtering/FastBilateral/ using the v3 whitelist filter-repo pipeline. Upstream repo: https://github.com/InsightSoftwareConsortium/ITKFastBilateral.git Upstream tip: 82a7180a49e939010043fc560de1a3020aa5ab8e Ingest date: 2026-04-28 Whitelist passes (git filter-repo): - --path include --path src --path test --path wrapping - --path CMakeLists.txt --path itk-module.cmake - --to-subdirectory-filter Modules/Filtering/FastBilateral - --prune-empty always - (if present) second pass: invert CTestConfig.cmake Outcome: 32 upstream commits -> 13 surviving; 4 distinct authors preserved; git blame walks across the merge boundary to original authors. Content-link inventory: .md5=0 .shaNNN=0 .cid=4 Primary author: Pablo Hernandez-Cerdan <pablo.hernandez.cerdan@outlook.com> Co-authored-by: Dženan Zukić <dzenan.zukic@kitware.com> Co-authored-by: Hans J. Johnson <hans-johnson@uiowa.edu> Co-authored-by: Hans Johnson <hans-johnson@uiowa.edu>
The upstream remote module's top-level CMakeLists.txt carried a dual-mode preamble that supports both standalone (find_package(ITK) + ITKModuleExternal) and in-tree (itk_module_impl) builds. Once the module lives under Modules/Filtering/, only the in-tree branch is reachable -- the standalone branch is dead code, the cmake_minimum_required and project() lines duplicate what ITK's root CMake already owns. Collapse the file to a single itk_module_impl() call, matching every other in-tree Modules/<Group>/<Name>/ module. Per the standalone-build cleanup convention documented in Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md under "Standalone-build boilerplate cleanup (modes A/B)" and applied previously to GenericLabelInterpolator (PR InsightSoftwareConsortium#6135) and MGHIO (PR InsightSoftwareConsortium#6137).
The module now lives at Modules/Filtering/FastBilateral/. The configure-time fetch declaration in Modules/Remote/ would otherwise clone the (soon-to-be-archived) upstream and double-define the module. Mirrors the in-tree transition completed for GenericLabelInterpolator (PR InsightSoftwareConsortium#6135) and MGHIO (PR InsightSoftwareConsortium#6137).
Add -DModule_FastBilateral:BOOL=ON to the Pixi-Cxx configure-ci task so the in-tree module is exercised by the default CI matrix, matching the policy applied to AnisotropicDiffusionLBR, Montage, GenericLabelInterpolator, MGHIO, and StructuralSimilarity. The module declares EXCLUDE_FROM_DEFAULT in its itk-module.cmake, so this opt-in is required for it to be built and tested in CI.
The upstream itk-module.cmake reads README.rst at configure time to populate the module DESCRIPTION. Only the whitelisted code/test paths crossed the ingest boundary (no README.rst), so configure would fail with "file does not exist". Inline the description as a literal string consistent with the AnisotropicDiffusionLBR, GenericLabelInterpolator, and MGHIO ingest patterns, and add a Modules/Filtering/FastBilateral/README.md pointer to the archived upstream repository.
6 tasks
github-actions
Bot
added
type:Infrastructure
hjmjohnson
force-pushed
the
ingest-FastBilateral
branch
2 times, most recently
from
1a80e79 to
8ecdffe
Compare
Member
|
@hjmjohnson All the integrated remote modules are still going to be off by default. Do we have any planned idea how they can be tested? Do we need "smart" modules enabled on some CI builds? If we turn on code coverage on valgrind or other additional check I suspect we may find additional issues too. Just wanted to talk through this. |
Member
Author
For each ingested module there should be a commit "ENH: Enable FastBilateral in CI via configure-ci". So far, they have had minimal impact on build times and would be easy to revert in the future. I want to ensure that they are tested on import, and we can refine later on. My preference is to continue to push to get these in. Addressing valgrind or coverage issues across all ITK code is so much easier than addressing 60 one-offs. |
This comment was marked as resolved.
This comment was marked as resolved.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Member
|
This PR also has linearized history, without the merge commit. The rest looks fine. |
This was referenced Apr 28, 2026
Member
Author
ARRG! I thought I made the ruleset robust enough to avoid this. I'm adding more guiderails to see if I can get the right demands in place to accomplish this. I might need more intermediate steps to get this accomplished. |
Apply ITK's current pre-commit formatting set to the ingested CMake and wrapping files inherited from upstream. The upstream module pre-dates ITK's current gersemi/eof-fixer adoption; running them now lets the in-tree pre-commit gate pass cleanly on every push. Mode A step 6 of the ingestion strategy (Utilities/Maintenance/RemoteModuleIngest/INGESTION_STRATEGY.md).
hjmjohnson
force-pushed
the
ingest-FastBilateral
branch
from
8ecdffe to
dd21525
Compare
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
Apply tip-of-branch fixups for style/quality issues greptile flagged
on upstream-inherited code, in keeping with the ingestion strategy of
not rewriting upstream history for non-ghostflow concerns:
- itkFastBilateralImageFilter.h:
- template <class T> -> template <typename T>
- virtual ~Filter() {} -> ~Filter() override = default
- drop redundant 'const' from itkGetConstMacro(DomainSigma, ...)
- itkFastBilateralImageFilter.hxx:
- mark immutable locals (padding, variance, maxWidth) const
- itkFastBilateralImageFilterTest2.cxx:
- return EXIT_FAILURE instead of -1 / -2
- fix misleading "Test itkSetVectorMacro" comment (the macro called
is itkSetMacro on a fixed-size array)
- itkFastBilateralImageFilterTest3.cxx:
- return EXIT_FAILURE instead of -1
- move writer->Update() inside the try-catch block so I/O
exceptions are reported the same way as filter exceptions
Member
Author
|
Addressed greptile's P2 style/quality findings in d3a0668 as a tip-of-branch Header (
Implementation (
Tests
Out of scope (left for a follow-up — not regressions, just upstream-inherited):
Note on @greptileai re-review. |
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 28, 2026
The normalize-ingest-commits.py script replays each non-merge upstream commit via cherry-pick and skips merges entirely. The result loses the upstream merge topology, which the strategy doc has always mandated as preserved. Reviewers flagged this on PRs InsightSoftwareConsortium#6135, InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6159, and InsightSoftwareConsortium#6161 before it became a rule violation. This commit: 1. Adds a hard refuse-to-run guard to normalize-ingest-commits.py (exit 3 with deprecation notice unless --i-understand-this-linearizes is passed). The guard names the replacement and points at the strategy doc. 2. Adds rewrite-history-merge-preserving.py — a git-filter-repo driven rewriter that walks every commit (merges included) and applies uniform text-blob normalization (trailing-whitespace strip + EOF-newline fix). Implements the user-mandated Phase 1 reference / Phase 2 traversal / Phase 3 verification architecture. 3. Updates INGESTION_STRATEGY.md with the "Linearization is forbidden" subsection, an allowed-tool table for per-commit re-formatting, and the new operator check that the merge count satisfies 1 + UPSTREAM_INTERNAL_MERGES. Language-specific formatters (clang-format, gersemi) are intentionally NOT applied per blob — they require the destination's current config, and applying them retroactively across upstream history would produce spurious diffs. The recommended pattern is a single "STYLE: Apply current formatters" commit at the rewritten tip after Phase 2 succeeds.
hjmjohnson
merged commit 2722d52
into
InsightSoftwareConsortium:main
16 of 18 checks passed
Member
|
Awesome! 🥳 |
hjmjohnson
added a commit
to InsightSoftwareConsortium/ITKFastBilateral
that referenced
this pull request
Apr 29, 2026
The FastBilateral module has been ingested into ITK main via InsightSoftwareConsortium/ITK#6159 (merge commit 2722d525d7). Remove the now-duplicated content from this repo's tree tip and add MIGRATION_README.md pointing future visitors at the authoritative in-tree location. Pre-ingest git history is preserved for blame and archaeology; this repo will be marked ARCHIVED in GitHub Settings after merge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Re-does @dzenanz's #5134 using the v3 whitelist-based ingestion strategy now applied to all incoming remote-module consolidations. Resulting tree is
Modules/Filtering/FastBilateral/(its own module, the same shape used forGenericLabelInterpolatorin #6135 andMGHIOin #6137), not merged intoModules/Filtering/ImageFeature/as in the original PR.Closes the upstream side of #6060 for
FastBilateral. After this lands, a follow-up PR onInsightSoftwareConsortium/ITKFastBilateralwill delete the whitelisted files and addMIGRATION_README.mdso the upstream repo can be archived (same pattern as ITKMGHImageIO#50 and ITKGenericLabelInterpolator#43).Topology
Mode A merge per the strategy: the ingest commit
9e13c4ac89is a--no-ff --allow-unrelated-historiesmerge of the filter-repo'd upstream history (13 surviving commits, 16 whitelisted files, 4.cidbaselines already CID-form) into ITKmain.git blamewalks across the merge into the original upstream authors (zukic, kruegere, others).What lives where after this PR
Modules/Filtering/FastBilateral/include/itkFastBilateralImageFilter.{h,hxx}Modules/Filtering/FastBilateral/test/itkFastBilateralImageFilterTest{,2,3}.cxxModules/Filtering/FastBilateral/test/Baseline/*.cidModules/Filtering/FastBilateral/test/Input/cake_easy.png{.cid,}Modules/Filtering/FastBilateral/wrapping/itkFastBilateralImageFilter.wrapModules/Filtering/FastBilateral/CMakeLists.txtitk_module_impl()Modules/Filtering/FastBilateral/itk-module.cmakeModules/Filtering/FastBilateral/README.mdModules/Remote/FastBilateral.remote.cmakepyproject.toml(configure-ci task)+-DModule_FastBilateral:BOOL=ONThe module declares
EXCLUDE_FROM_DEFAULT(it carries Python-wrapped tests with downloaded baselines, opt-in for CI), matchingAnisotropicDiffusionLBR/GenericLabelInterpolator/MGHIO.Differences from #5134
Modules/Filtering/ImageFeature/(merged in)Modules/Filtering/FastBilateral/(own module)git blameModules/Remote/FastBilateral.remote.cmakeLocal verification
pixi run -e cxxconfigure with-DModule_FastBilateral=ONsucceeds (exit 0).itkFastBilateralImageFilterTest{,2,3}.cxxtranslation units compile cleanly under the project's-Wshadow -Wall -Wextraset.libitkgdcmMSFFsymbol (a host-environment GDCM/libiconv issue independent of this PR); CI will validate the full link +ctest -R FastBilateralpass.