Skip to content

ENH: Ingest ITKHigherOrderAccurateGradient into Modules/Filtering#6242

Open
hjmjohnson wants to merge 44 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:ingest-HigherOrderAccurateGradient
Open

ENH: Ingest ITKHigherOrderAccurateGradient into Modules/Filtering#6242
hjmjohnson wants to merge 44 commits intoInsightSoftwareConsortium:mainfrom
hjmjohnson:ingest-HigherOrderAccurateGradient

Conversation

@hjmjohnson
Copy link
Copy Markdown
Member

Ingest the standalone remote module
InsightSoftwareConsortium/ITKHigherOrderAccurateGradient
into Modules/Filtering/HigherOrderAccurateGradient/. Header-only filter
that computes higher-order accurate centered finite differences of
arbitrary even order on N-dimensional scalar images. In-tree under
EXCLUDE_FROM_DEFAULT.

Built directly on upstream/main via the v4 ingestion pipeline (#6204).

Commits
# Subject
1 ENH: Ingest ITKHigherOrderAccurateGradient into Modules/Filtering (Mode-A merge)
2 DOC: Add HigherOrderAccurateGradient README pointing at upstream
3 COMP: Remove HigherOrderAccurateGradient .remote.cmake (in-tree)
4 ENH: Enable Module_HigherOrderAccurateGradient in configure-ci

Merge topology preserved: 3 merge commits, 42 commits unique to this PR.

Phase B follow-up

After merge, archive the upstream repo via the v4 archive script:

Utilities/Maintenance/RemoteModuleIngest/archive-remote-module.sh \
    HigherOrderAccurateGradient

thewtex and others added 30 commits October 12, 2010 16:04
Why limit ourselves to 2nd order?  Found a nice paper.
A little less confusing since the radius of the kernel corresponds to a 2N
Taylor series accuracy.
Otherwise test will fail.
Will refactor for ITKv4 as a Module.
Double underscore is reserved for compiler defined preprocessor variables.
Provide initial conversion of features to ITKv5.
Provide remove virtual and override

Use clang-tidy to add ITK_OVERRIDE, and to remove
redundant virtual on functions.

cd ../ITK;
clang-tidy -p ITK-clangtidy $find Modules/[A-J]*  -name *.cxx |fgrep -v ThirdParty) -checks=-*,modernize-use-override  -header-filter=.* -fix
clang-tidy -p ITK-clangtidy $(find Modules/[K-Z]*  -name *.cxx |fgrep -v ThirdParty) -checks=-*,modernize-use-override  -header-filter=.* -fix

https://stackoverflow.com/questions/39932391/virtual-override-or-both-c

When you override a function you don't technically need to write either virtual
or override.

The original base class declaration needs the keyword virtual to mark it as virtual.

In the derived class the function is virtual by way of having the ¹same type as
the base class function.

However, an override can help avoid bugs by producing a compilation error when
the intended override isn't technically an override. E.g. that the function
type isn't exactly like the base class function. Or that a maintenance of the
base class changes that function's type, e.g. adding a defaulted argument.

In the same way, a virtual keyword in the derived class can make such a bug
more subtle, by ensuring that the function is still is virtual in further
derived classes.

So the general advice is,

virtual for the base class function declaration.
This is technically necessary.

Use override (only) for a derived class' override.
This helps with maintenance.
git grep -l "ITK_OVERRIDE" |   fgrep -v itk_compiler_detection.h | fgrep -v CMakeLists.txt |fgrep -v .cmake |   xargs sed -i '' -e "s/ITK_OVERRIDE/override/g"
Use static constexpr directly now that C++11 conformance
is required by all compilers.

:%s/itkStaticConstMacro *( *\([^,]*\),[ \_s]*\([^,]*\),\_s*\([^)]*\)) */static constexpr \2 \1 = \3/ge
== http://en.cppreference.com/w/cpp/language/type_alias ==

Type alias is a name that refers to a previously defined type (similar
to typedef).

A type alias declaration introduces a name which can be used as a
synonym for the type denoted by type-id. It does not introduce a new
type and it cannot change the meaning of an existing type name. There is
no difference between a type alias declaration and typedef declaration.
This declaration may appear in block scope, class scope, or namespace
scope.

== https://www.quora.com/Is-using-typedef-in-C++-considered-a-bad-practice ==

While typedef is still available for backward compatibility, the new
Type Alias syntax 'using Alias = ExistingLongName;' is more consistent
with the flow of C++ than the old typedef syntax 'typedef
ExistingLongName Alias;', and it also works for templates (Type alias,
alias template (since C++11)), so leftover 'typedef' aliases will differ
in style from any alias templates.
Use constexpr for constant numeric literals.
Move `ITK_DISALLOW_COPY_AND_ASSIGN` calls to public section following
the discussion in
https://discourse.itk.org/t/noncopyable

If legacy (pre-macro) copy and assing methods existed, subsitute them
for the `ITK_DISALLOW_COPY_AND_ASSIGN` macro.
Add CircleCI configuration files and badges.
Instantiated types should be compatible with class template
specification.

Types should correspond to selected wrapping types.
ENH: Add CI configuration files.
As discussed in:
http://review.source.kitware.com/#/c/12655/

the use of the template keyword "class" was substituted by "typename" in
the toolkit for the reasons stated in that topic.
…ently

STYLE: Use "typename" for template parameters.
This check replaces default bodies of special member functions with
= default;. The explicitly defaulted function declarations enable more
opportunities in optimization, because the compiler might treat
explicitly defaulted functions as trivial.

Additionally, the C++11 use of = default more clearly expreses the
intent for the special member functions.
Converts a default constructor’s member initializers into the new
default member initializers in C++11. Other member initializers that match the
default member initializer are removed. This can reduce repeated code or allow
use of ‘= default’.
hjmjohnson and others added 12 commits February 23, 2020 09:46
This check replaces default bodies of special member functions with
= default;. The explicitly defaulted function declarations enable more
opportunities in optimization, because the compiler might treat
explicitly defaulted functions as trivial.

Additionally, the C++11 use of = default more clearly expreses the
intent for the special member functions.
The mission of NumFOCUS is to promote open
practices in research, data, and scientific
computing.

https://numfocus.org
Fixes changes made in InsightSoftwareConsortium#2053. ITK_DISALLOW_COPY_AND_ASSIGN will be used if ITK_FUTURE_LEGACY_REMOVE=OFF.
A ';' is required at the end of macros in a future version 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).
Fix module description: remove hanging sentence part.
Mode-A merge of upstream ITKHigherOrderAccurateGradient into ITK
proper at Modules/Filtering/HigherOrderAccurateGradient/, ingested
via the v4 pipeline (whitelist filter-repo + per-blob sanitize +
no-ff merge).  Header-only filter providing higher-order accurate
numerical-derivative and gradient image filters.

Upstream:
  https://github.com/InsightSoftwareConsortium/ITKHigherOrderAccurateGradient

The upstream repository will be archived read-only after this PR
merges; it remains reachable at the URL above for historical
reference.
@github-actions github-actions Bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Enhancement Improvement of existing methods or implementation area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Filtering Issues affecting the Filtering module area:Remotes Issues affecting the Remote module labels May 9, 2026
Replace the set(DOCUMENTATION "...") + DESCRIPTION "${DOCUMENTATION}"
indirection with a static one-liner literal.

itk_module() is a CMake macro, so ${ARGN} re-tokenizes its arguments
and list-splits any embedded ";". A future edit adding a semicolon or
"[" to the DOCUMENTATION string would silently produce spurious
"Unknown argument" AUTHOR_WARNINGs from CMake/ITKModuleMacros.cmake:111
on every configure (see PRs InsightSoftwareConsortium#6220, InsightSoftwareConsortium#6245).

The v4 ingestion pipeline (PR InsightSoftwareConsortium#6204) enforces this via
sanitize-history.py:patch_dynamic_description.
@hjmjohnson
Copy link
Copy Markdown
Member Author

@greptile please review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 9, 2026

Greptile Summary

Ingests the ITKHigherOrderAccurateGradient remote module into Modules/Filtering/HigherOrderAccurateGradient/ as a first-class in-tree module and removes the corresponding .remote.cmake entry. The module is registered as EXCLUDE_FROM_DEFAULT and enabled explicitly in the CI configure step.

  • New headers: HigherOrderAccurateDerivativeOperator, HigherOrderAccurateDerivativeImageFilter, and HigherOrderAccurateGradientImageFilter — all header-only, implementing centered finite-difference kernels of arbitrary even order on N-D scalar images.
  • Removed: Modules/Remote/HigherOrderAccurateGradient.remote.cmake; pyproject.toml gains the corresponding -DModule_HigherOrderAccurateGradient:BOOL=ON flag.

Confidence Score: 4/5

Safe to merge after addressing the zero-accuracy guard; all other code is a faithful in-tree ingestion of a working remote module.

The coefficient generator writes past the end of the coefficient vector when m_OrderOfAccuracy is 0 — unsigned underflow on order-1 produces a near-UINT_MAX index, causing a definite crash. The default value of 2 protects typical users, but the API accepts 0 with no diagnostic.

itkHigherOrderAccurateDerivativeOperator.hxx — GenerateFirstOrderCoefficients needs an order==0 guard.

Important Files Changed

Filename Overview
Modules/Filtering/HigherOrderAccurateGradient/include/itkHigherOrderAccurateDerivativeOperator.hxx GenerateFirstOrderCoefficients() has an out-of-bounds access when m_OrderOfAccuracy==0: coeff[order+1] and coeff[order-1] are written to index 1 and UINT_MAX respectively on a length-1 vector.
Modules/Filtering/HigherOrderAccurateGradient/itk-module.cmake ITKImageGradient and ITKImageIntensity are listed in DEPENDS but only needed by the test executables; ITKSmoothing (for NeighborhoodOperatorImageFilter) is absent from DEPENDS.
Modules/Filtering/HigherOrderAccurateGradient/include/itkHigherOrderAccurateDerivativeImageFilter.hxx Mini-pipeline implementation looks correct; spacing guard, progress accumulator, and region-padding are all present.
Modules/Filtering/HigherOrderAccurateGradient/include/itkHigherOrderAccurateGradientImageFilter.hxx DynamicThreadedGenerateData follows the same slice-based operator pattern as ITK's stock GradientImageFilter; direction handling looks correct.
Modules/Remote/HigherOrderAccurateGradient.remote.cmake Remote cmake entry correctly deleted now that the module is ingested in-tree.
pyproject.toml Correctly enables Module_HigherOrderAccurateGradient in the CI configure line.

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class NeighborhoodOperator {
        <<abstract>>
        +CreateDirectional()
        +FlipAxes()
        +ScaleCoefficients()
    }
    class HigherOrderAccurateDerivativeOperator {
        -m_Order : unsigned int
        -m_OrderOfAccuracy : unsigned int
        +SetOrder()
        +SetOrderOfAccuracy()
        #GenerateCoefficients() CoefficientVector
        -GenerateFirstOrderCoefficients() CoefficientVector
    }
    class ImageToImageFilter {
        <<abstract>>
    }
    class HigherOrderAccurateDerivativeImageFilter {
        -m_Order : unsigned int
        -m_OrderOfAccuracy : unsigned int
        -m_Direction : unsigned int
        -m_UseImageSpacing : bool
        +GenerateData()
        +GenerateInputRequestedRegion()
    }
    class HigherOrderAccurateGradientImageFilter {
        -m_UseImageSpacing : bool
        -m_UseImageDirection : bool
        -m_OrderOfAccuracy : unsigned int
        +DynamicThreadedGenerateData()
        +GenerateInputRequestedRegion()
    }
    NeighborhoodOperator <|-- HigherOrderAccurateDerivativeOperator
    ImageToImageFilter <|-- HigherOrderAccurateDerivativeImageFilter
    ImageToImageFilter <|-- HigherOrderAccurateGradientImageFilter
    HigherOrderAccurateDerivativeImageFilter ..> HigherOrderAccurateDerivativeOperator : uses
    HigherOrderAccurateGradientImageFilter ..> HigherOrderAccurateDerivativeOperator : uses
Loading

Reviews (2): Last reviewed commit: "STYLE: Address Greptile review on Higher..." | Re-trigger Greptile

@hjmjohnson hjmjohnson marked this pull request as ready for review May 9, 2026 14:34
- Drop `== true` boolean comparisons (itkHigherOrderAccurateDerivativeImageFilter.hxx,
  itkHigherOrderAccurateGradientImageFilter.hxx).
- Collapse stray blank line between `Ctor()` signature and `= default;` in
  Derivative filter, GradientImageFilter ctor, and DerivativeOperator ctor.
- Document SetOrder() limitation: only `order == 1` is implemented;
  GenerateCoefficients() throws on any other value.
- Rename misnamed wrap file
  itkHigherOrderAccurateGradientmageFilter.wrap ->
  itkHigherOrderAccurateGradientImageFilter.wrap.
@hjmjohnson hjmjohnson force-pushed the ingest-HigherOrderAccurateGradient branch from ff64672 to b2712b1 Compare May 9, 2026 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Filtering Issues affecting the Filtering module area:Python wrapping Python bindings for a class area:Remotes Issues affecting the Remote module type:Enhancement Improvement of existing methods or implementation type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants