Ingest ITKAdaptiveDenoising into Modules/Filtering#6235
Ingest ITKAdaptiveDenoising into Modules/Filtering#6235hjmjohnson wants to merge 39 commits intoInsightSoftwareConsortium:mainfrom
Conversation
Use the `ITK_DISALLOW_COPY_AND_ASSIGN` macro to enhance consistency across the toolkit when disallowing the copy constructor and the assign operator. Move the `ITK_DISALLOW_COPY_AND_ASSIGN` calls to the `public` section following the discussion in https://discourse.itk.org/t/itk-disallow-copy-and-assign/648
…gnMacro COMP: Use and move ITK_DISALLOW_COPY_AND_ASSIGN calls to public section.
Use strongly type enums. Set `AdaptiveDenoising_LIBRARIES` for external builds too derived from the changes involved by the presence of a C++ implementation file.
ENH: Use strongly typed enums
Test the similarity metric. Take advantage of the commit to improve the test style to make a better use of the ITK macros.
ENH: Test the similarity metric
Replace the deprecated ITK_DISALLOW_COPY_AND_ASSIGN with new better descriptive name ITK_DISALLOW_COPY_AND_MOVE.
For consistency, a ';' will be required in a future version of ITK.
COMP: Update to new ITK_DISALLOW_COPY_AND_MOVE macro
Use of SetUseImageSpacingOn() is deprecated in the future. 'Set' prefixed functions have an argument that is used to set an ivar. UseImageSpacingOn() is also valid, but is less compatible for older versions of ITK.
The ability to include either .h or .hxx files as header files required recursively reading the .h files twice. The added complexity is unnecessary, costly, and can confuse static analysis tools that monitor header guardes (due to reaching the maximum depth of recursion limits for nested #ifdefs in checking).
…ludes COMP: Remove inclusion of .hxx files as headers
Images with small values have standard deviations that could be smaller than 1.0, but be perfectly valid. For the case when the standard deviation is zero, set all the z-score values to zero.
…utations Fix small value zscore computations
ENH: Bump ITK and change http to https
When preparing for the future with ITK by setting ITK_FUTURE_LEGACY_REMOVE:BOOL=ON ITK_LEGACY_REMOVEBOOL=ON The future preferred macro should be used │ - itkTypeMacro │ + itkOverrideGetNameOfClassMacro
Use static constexpr directly now that C++11 conformance is required by all compilers. :%s/itkStaticConstMacro *( *\([^,]*\),[ \_s]*\([^,]*\),\_s*\([^)]*\)) */static constexpr \2 \1 = \3/ge
…ix-legacy-remove COMP: Use modern macro for name of class
github-actions
Bot
added
type:Infrastructure
hjmjohnson
force-pushed
the
ingest-AdaptiveDenoising
branch
from
7bff92d to
d2e8ffa
Compare
hjmjohnson
changed the title
hjmjohnson
force-pushed
the
ingest-AdaptiveDenoising
branch
2 times, most recently
from
84fa925 to
67858a2
Compare
Phase A v4 ingest of the standalone remote module ntustison/ITKAdaptiveDenoising into Modules/Filtering/AdaptiveDenoising/. Whitelisted upstream history was sanitized via the v4 pipeline (see InsightSoftwareConsortium#6204), then stripped of all raw .nrrd test fixtures via 'git filter-repo --invert-paths' so that no raw binary blobs land in ITK's git history. Sibling .cid stubs are added in a follow-up commit; their fetch path is published via ITKTestingData PR InsightSoftwareConsortium#48. Stacked on the pixi 0.68 multi-line cmd compatibility fix from InsightSoftwareConsortium#6236.
Test fixtures for the AdaptiveNonLocalMeansDenoising filter:
bafkreiafwnstu4vzkrl7gpt73tl3q53ywpdylbs2d4emezgzutj2thoyfy
r16slice.nrrd (test/Input)
bafkreigltzq46r5ojiiyel37egvlosnxxgyth6ppwckzjtuxyywtlijoii
r16denoised_mean_squares.nrrd (test/Baseline)
bafkreickjyvdwmq6eyz76e7cukkegm4dd75raimxj4pag2o6bkanvbofwm
r16denoised_pearson_correlation.nrrd (test/Baseline)
The blobs are pre-uploaded to ITKTestingData via PR InsightSoftwareConsortium#48; the
upstream remote module shipped these as raw .nrrd files inline in
git, which were stripped from the rewritten ingest history (no raw
binaries in ITK's history) and replaced by these .cid stubs at the
ingest tip. CIDs were computed via 'ipfs add --cid-version=1
--raw-leaves'.
hjmjohnson
force-pushed
the
ingest-AdaptiveDenoising
branch
from
67858a2 to
7417214
Compare
|
| Filename | Overview |
|---|---|
| Modules/Filtering/AdaptiveDenoising/include/itkAdaptiveNonLocalMeansDenoisingImageFilter.hxx | Flagship filter implementation; contains a thread-safety race condition where ThreadedGenerateData writes output and count-image pixels outside its assigned sub-region without synchronization, producing nondeterministic results on multi-core systems. |
| Modules/Filtering/AdaptiveDenoising/include/itkNonLocalPatchBasedImageFilter.hxx | Base class implementation; VectorizeImagePatch normalization falls through after zeroing elements when std-dev is near-zero, producing large/infinite values instead of zeros for any normalize=true caller. |
| Modules/Filtering/AdaptiveDenoising/include/itkAdaptiveNonLocalMeansDenoisingImageFilter.h | Filter header; uses old typedef style throughout while base class uses modern using aliases — inconsistent with ITK coding standards. |
| Modules/Filtering/AdaptiveDenoising/include/itkVarianceImageFilter.h | Local variance filter header; has incorrect Doxygen ("averaging filter", "linear filter"), a typo ("variacne"), and uses typedef instead of using aliases. |
| Modules/Filtering/AdaptiveDenoising/include/itkNonLocalPatchBasedImageFilter.h | Base class header; clean modern using aliases, enum class with streaming support, legacy aliases guarded by ITK_LEGACY_REMOVE — no issues found. |
| Modules/Filtering/AdaptiveDenoising/itk-module.cmake | Module declaration with correct DEPENDS, COMPILE_DEPENDS, TEST_DEPENDS, EXCLUDE_FROM_DEFAULT, and ENABLE_SHARED — looks correct. |
| Modules/Filtering/AdaptiveDenoising/test/CMakeLists.txt | Two test cases registered with DATA{} content-link references for .cid stubs — correctly follows ITK ExternalData convention. |
| Modules/Remote/AdaptiveDenoising.remote.cmake | Remote module descriptor correctly removed now that the module is in-tree; deletion is the right action. |
| pyproject.toml | Adds -DModule_AdaptiveDenoising:BOOL=ON to the CI configure command so the new module is exercised in CI builds. |
| Modules/Filtering/AdaptiveDenoising/wrapping/CMakeLists.txt | Wrapping order declared correctly: VarianceImageFilter → NonLocalPatchBasedImageFilter → AdaptiveNonLocalMeansDenoisingImageFilter. |
| Modules/Filtering/AdaptiveDenoising/test/itkAdaptiveNonLocalMeansDenoisingImageFilterTest.cxx | Test exercises both similarity metrics, verifies all public API getters/setters, and uses ITK_TRY_EXPECT_NO_EXCEPTION — good coverage of the public interface. |
Reviews (1): Last reviewed commit: "ENH: Enable Module_AdaptiveDenoising in ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (unsigned int n = 0; n < neighborhoodPatchSize; n++) | ||
| { | ||
| IndexType neighborhoodPatchIndex = centerIndex + neighborhoodPatchOffsetList[n]; | ||
| if (!targetImageRegion.IsInside(neighborhoodPatchIndex)) | ||
| { | ||
| continue; | ||
| } | ||
| if (this->m_UseRicianNoiseModel) | ||
| { | ||
| weightedAverageIntensities[n] += maxWeight * itk::Math::sqr(inputImage->GetPixel(neighborhoodPatchIndex)); | ||
| } | ||
| else | ||
| { | ||
| weightedAverageIntensities[n] += maxWeight * inputImage->GetPixel(neighborhoodPatchIndex); | ||
| } | ||
| } | ||
| sumOfWeights += maxWeight; | ||
|
|
||
| if (sumOfWeights > itk::NumericTraits<RealType>::ZeroValue()) | ||
| { | ||
| for (unsigned int n = 0; n < neighborhoodPatchSize; n++) | ||
| { | ||
| IndexType neighborhoodPatchIndex = centerIndex + neighborhoodPatchOffsetList[n]; | ||
| if (!targetImageRegion.IsInside(neighborhoodPatchIndex)) | ||
| { | ||
| continue; | ||
| } | ||
| typename OutputImageType::PixelType estimate = outputImage->GetPixel(neighborhoodPatchIndex); | ||
| estimate += (weightedAverageIntensities[n] / sumOfWeights); | ||
|
|
||
| outputImage->SetPixel(neighborhoodPatchIndex, estimate); | ||
| this->m_ThreadContributionCountImage->SetPixel( | ||
| neighborhoodPatchIndex, this->m_ThreadContributionCountImage->GetPixel(neighborhoodPatchIndex) + 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
ThreadedGenerateData iterates over pixels in the per-thread region but writes outputImage and m_ThreadContributionCountImage at centerIndex + patchOffset, which can land in another thread's region. The read-modify-write sequences (GetPixel + SetPixel) on those shared images are not atomic, so concurrent threads silently corrupt each other's accumulations. The AfterThreadedGenerateData then divides the accumulated output by the corrupted counts, producing incorrect denoising results on any multi-core system. The same pattern appears for m_RicianBiasImage writes in the Rician-correction loop above (lines 234–251).
The canonical ITK fix is to split accumulation (one per-thread intermediate buffer) and merge in AfterThreadedGenerateData, or to use a mutex around the GetPixel/SetPixel pair.
| if (standardDeviation < NumericTraits<RealType>::epsilon()) | ||
| { | ||
| for (auto & it : patchVector) | ||
| { | ||
| it = 0.; | ||
| } | ||
| } | ||
| const auto inv_standardDeviation = 1. / standardDeviation; | ||
| for (auto & it : patchVector) | ||
| { | ||
| it = (it - mean) * inv_standardDeviation; | ||
| } | ||
| } |
There was a problem hiding this comment.
When standardDeviation < epsilon, the intent is to return a zero vector. The code zeroes every element in the loop (lines 121–124) but then falls through unconditionally to compute inv_standardDeviation = 1.0 / standardDeviation (a tiny or denormalized divisor) and applies it = (0 - mean) * inv_standardDeviation. The result is -mean / epsilon, which can be orders of magnitude above the expected zero — or even ±Inf. Any callers of VectorizeImagePatch with normalize=true (e.g. future subclasses) will silently receive corrupted patch vectors. An early return after zeroing, or wrapping the normalization block in an else, is needed.
| /** \class VarianceImageFilter | ||
| * \brief Applies an averaging filter to an image | ||
| * | ||
| * Computes an image where a given pixel is the variance value of the | ||
| * the pixels in a neighborhood about the corresponding input pixel. | ||
| * | ||
| * A variacne filter is one of the family of linear filters. | ||
| * | ||
| * \ingroup AdaptiveDenoising | ||
| */ |
There was a problem hiding this comment.
| /** \class VarianceImageFilter | |
| * \brief Applies an averaging filter to an image | |
| * | |
| * Computes an image where a given pixel is the variance value of the | |
| * the pixels in a neighborhood about the corresponding input pixel. | |
| * | |
| * A variacne filter is one of the family of linear filters. | |
| * | |
| * \ingroup AdaptiveDenoising | |
| */ | |
| /** \class VarianceImageFilter | |
| * \brief Applies a variance filter to an image | |
| * | |
| * Computes an image where a given pixel is the sample variance of the | |
| * pixels in a neighborhood about the corresponding input pixel. | |
| * | |
| * A variance filter is a nonlinear (quadratic) neighborhood filter. | |
| * | |
| * \ingroup AdaptiveDenoising | |
| */ |
| /** Standard class typedefs. */ | ||
| typedef AdaptiveNonLocalMeansDenoisingImageFilter Self; | ||
| typedef NonLocalPatchBasedImageFilter<TInputImage, TOutputImage> Superclass; | ||
| typedef SmartPointer<Self> Pointer; | ||
| typedef SmartPointer<const Self> ConstPointer; | ||
|
|
||
| /** Runtime information support. */ | ||
| itkOverrideGetNameOfClassMacro(AdaptiveNonLocalMeansDenoisingImageFilter); | ||
|
|
||
| /** Standard New method. */ | ||
| itkNewMacro(Self); | ||
|
|
||
| /** ImageDimension constants */ | ||
| static constexpr unsigned int ImageDimension = TInputImage::ImageDimension; | ||
|
|
||
| /** Some convenient typedefs. */ | ||
| typedef TInputImage InputImageType; | ||
| typedef typename InputImageType::PixelType InputPixelType; | ||
| typedef TOutputImage OutputImageType; | ||
| typedef typename Superclass::RegionType RegionType; | ||
|
|
||
| typedef TMaskImage MaskImageType; | ||
| typedef typename MaskImageType::PixelType MaskPixelType; | ||
| typedef typename MaskImageType::PixelType LabelType; | ||
|
|
||
| typedef typename Superclass::RealType RealType; | ||
| typedef typename Superclass::RealImageType RealImageType; | ||
| typedef typename Superclass::RealImagePointer RealImagePointer; | ||
| typedef typename Superclass::IndexType IndexType; | ||
|
|
||
| typedef typename Superclass::ConstNeighborhoodIteratorType ConstNeighborhoodIteratorType; | ||
| typedef typename Superclass::NeighborhoodRadiusType NeighborhoodRadiusType; | ||
| typedef typename Superclass::NeighborhoodOffsetType NeighborhoodOffsetType; | ||
| typedef typename Superclass::NeighborhoodOffsetListType NeighborhoodOffsetListType; | ||
|
|
||
| typedef GaussianOperator<RealType> ModifiedBesselCalculatorType; |
There was a problem hiding this comment.
typedef style instead of using throughout this header and itkVarianceImageFilter.h
All type aliases in this header use the old typedef form rather than the using syntax required by ITK's enforced code style. The base class header itkNonLocalPatchBasedImageFilter.h already uses using consistently. The same issue appears in itkVarianceImageFilter.h (lines 48–71). Both files should be updated to using Foo = Bar style for consistency with the rest of the module and ITK's clang-tidy rules.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ImageRegionIteratorWithIndex<OutputImageType> ItO(this->GetOutput(), this->GetOutput()->GetRequestedRegion()); | ||
| ImageRegionConstIterator<RealImageType> ItL(this->m_ThreadContributionCountImage, | ||
| this->m_ThreadContributionCountImage->GetRequestedRegion()); | ||
| this->m_ThreadContributionCountImage->GetRequestedRegion()); |
| set(AdaptiveDenoising_SRCS | ||
| itkNonLocalPatchBasedImageFilterEnums.cxx | ||
| ) | ||
| set(AdaptiveDenoising_SRCS itkNonLocalPatchBasedImageFilterEnums.cxx) |
dzenanz
changed the title
5 participants
Ingest the standalone remote module
ntustison/ITKAdaptiveDenoisinginto
Modules/Filtering/AdaptiveDenoising/. Patch-based adaptivenon-local-means denoising is now in-tree under
EXCLUDE_FROM_DEFAULT.Built directly on
upstream/main; the v4 ingestion guidelinesdefined in #6204 (sanitize-history pass, deny-pattern pass,
whitelist-only paths, ghostflow-clean per-commit subjects) were
applied locally to produce this PR's content, but this PR does
not depend on #6204 merging first.
Test data is gated on
InsightSoftwareConsortium/ITKTestingData#48,which uploads the 3 fixture blobs the new tests fetch via
.cidstubs. Closes the consolidation work of issue #6060 for this
module.
Stacked on #6236 (pixi 0.68 multi-line cmd compatibility fix). The visible diff includes #6236's
pyproject.tomlline-continuation changes until that PR merges; once it lands, this branch can be rebased ontomain(usinggit rebase --rebase-mergesto keep the 9 ingest merges intact) and the pyproject.toml hunks will drop out.No raw binary blobs in ITK history
The upstream module shipped its 3 test fixtures
(
r16slice.nrrd,r16denoised_mean_squares.nrrd,r16denoised_pearson_correlation.nrrd) inline in git rather thanvia the ITK ExternalData / content-link convention. AGENTS.md
decision 3 forbids merging an ingest with raw binary
test/assets.
Rather than carry the binaries through the merge and then swap
them at the tip, the rewritten upstream history was passed
through
git filter-repo --path-glob '*.nrrd' --invert-pathsso that no commit reachable from this PR's HEAD adds a raw
.nrrd file:
The
.cidstubs are added in a single dedicated commit at thetip and point at the CID blobs published in
ITKTestingData PR #48.
Ingest results
include/,src/,test/,wrapping/,CMakeLists.txt,itk-module.cmake,LICENSE).nrrdfiles in history reachable from HEAD.cid, 0.md5, 0.shaNNN, 0 raw binariesexamples/directorygit blameLocal validation
pre-commit run --all-files: cleancmake -DModule_AdaptiveDenoising:BOOL=ON .: configures cleanitkAdaptiveNonLocalMeansDenoisingImageFilterTest.cxx.oand
itkNonLocalPatchBasedImageFilterEnums.cxx.obuild clean(16 pre-existing warnings about long subject lines from the
upstream's
itkGetConstMacro-generated names — not new).ITKTestingData#48
merging so the 3
.cidblobs are reachable through the standardmirror at fetch time.
Commits at PR tip (top → bottom)
ENH:Enable Module_AdaptiveDenoising in configure-ciCOMP:Remove AdaptiveDenoising .remote.cmake (in-tree)DOC:Add AdaptiveDenoising README pointing at upstreamENH:Add AdaptiveDenoising test data .cid stubs(3 stub files pointing at ITKTestingData PR ENH: Update CircleCI CMake version to 3.11.2 #48 blobs)
ENH:Ingest ITKAdaptiveDenoising into Modules/Filtering(merge commit
--no-ff --allow-unrelated-historiestosanitized + .nrrd-stripped filter-repo'd upstream history;
preserves 8 upstream merges and
git blameto original authors)Phase B (post-merge) — upstream archival
After this PR merges, run Phase B from a fresh worktree:
Utilities/Maintenance/RemoteModuleIngest/archive-remote-module.sh \ AdaptiveDenoisingPhase B publishes the upstream removal commit, promotes
MIGRATION_README.mdtoREADME.mdso the GitHub landing pagerenders the migration notice (per the
ingest-archive-readmerule), and flipsarchived=true. No upstream side-effects happen until that step.Refs: #6060