WIP: A ND Sobel implementation! #5718
Conversation
github-actions
bot
added
type:Testing
dzenanz
left a comment
dzenanz
left a comment
There was a problem hiding this comment.
Did we have dimensionality constraints somewhere? If so, we should lift them as part of this PR. Also, we probably want to add unit tests for higher dimensions.
Thanks for the reminder 😺 We would also have to generalize its So you see, it's really WIP! |
| // Standard Sobel definition: derivative = [-1, 0, 1], smoothing = [1, 2, 1]. | ||
| // Kernel for axis a is: K_a(x) = d[x_a] * Π_{j != a} s[x_j], with x_j ∈ {-1,0,1}. | ||
| inline std::vector<std::vector<int>> | ||
| make_nd_sobel_kernels(std::size_t N) |
There was a problem hiding this comment.
If this function were templated on N (aka VDimension), it could also be constexpr.
Then the test for N == 0 could also be a static assert instead of a runtime test.
There was a problem hiding this comment.
Thanks Hans! I just gave constexpr a try: WIP: Make make_nd_sobel_kernels constexpr (updated link)
Note that in this context, make_nd_sobel_kernels would just be an internal helper function of itk::SobelOperator<TPixel, VDimension, TAllocator>, so it already "knows" VDimension, and there is no need to pass it as parameter. VDimension can never be zero, so the assert isn't really necessary anyway.
Still: this is all WIP. Especially because I don't really understand the AI generated code, do you?
There was a problem hiding this comment.
In the mean time, I think I found an AI hallucination! Looking at the original AI generated code at #5702 (comment) This part appears useless:
// Precompute strides for row-major flattening (last dim fastest)
std::vector<std::size_t> stride(N, 1);
for (std::size_t i = 1; i < N; ++i) {
stride[N - 1 - i] = stride[N - i] * 3;
}The rest of the code does not make use of those strides! So it's unnecessary! I think I'll remove this part of code with the next force-push!
N-Dekker
force-pushed
the
Make-SobelOperator-3D-consistent
branch
2 times, most recently
from
54d23a1 to
730a117
Compare
|
FYI My intention is to squash most of these WIP commits before making the PR ready for review. (But before doing so, the PR should also offer a backward compatible legacy option, at least for 3D.) |
|
/azp run ITK.Linux.Python |
|
/azp run ITK.Linux |
N-Dekker
force-pushed
the
Make-SobelOperator-3D-consistent
branch
from
6ad490d to
4c20b2c
Compare
| const auto & kernel = sobelKernels[VDimension - 1 - direction]; | ||
| return CoefficientVector(kernel.cbegin(), kernel.cend()); |
There was a problem hiding this comment.
Why not simply kernel = sobelKernels[direction]? I still don't understand why the kernels returned by the AI generated function from #5702 (comment) are in the reverse order, compared by ITK 🤷
Aims to make the 3D coefficients more consistent with its 2D version, paving the way for an ND extension, as request by issue InsightSoftwareConsortium#5702 The new coefficients are more commonly used by other toolkits, including scipy, and correspond with https://en.wikipedia.org/wiki/Sobel_operator Removed the reference to "An Isotropic 3x3x3 Volume Gradient Operator", Sobel, 1995, as it cannot be found anymore.
Uses `make_nd_sobel_kernels` as literally copied from InsightSoftwareConsortium#5702 (comment)
Suggested by Hans Johnson. Replaced `std::vector` with `std::array`, in order to allow compile-time evaluation. Removed the check `if (N == 0)`, because it's not really a problem. For N == 0, the function returns an empty array, which is theoretically OK. In practice, VDimension is always greater than zero anyway. Removed the computation of "strides", as they appear unused. Possibly introduced by an AI hallucination...? Declared the local `kernels` variable in GenerateCoefficients() `static constexpr` to enforce that it is evaluated only once, during compilation.
Following C++ Core Guidelines, Jul 8, 2025, "Minimize the use of `break` and `continue` in loops", https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#res-continue Apparently, AI did not read the guidelines ;-)
Using the type alias from "itkIntTypes.h"
Renamed the function according to the Naming Conventions of ITK's Coding Style Guide, at https://github.com/InsightSoftwareConsortium/ITKSoftwareGuide/blob/0ca23823bb819fb6a4c85789f45d5e437e6a5c09/SoftwareGuide/Latex/Appendices/CodingStyleGuide.tex Also renamed local variables accordingly.
The symbols for product and "element of" were originally generated by AI.
Was accidentally removed by commit "WIP: Use AI generated make_nd_sobel_kernels in SobelOperator". The itkExceptionMacro message is more informative than `std::out_of_range` message from `std::array::at`.
Aims to address Darwin-Build12920 Python errors at https://open.cdash.org/viewBuildError.php?buildid=10942920 saying: Wrapping/Typedefs/itkSobelOperator.i:53: Error: Syntax error in input(3).
Another attempt to address Darwin-Build12920 Python errors at https://open.cdash.org/viewBuildError.php?buildid=10942920 saying: Wrapping/Typedefs/itkSobelOperator.i:53: Error: Syntax error in input(3).
Allows the user to specify whether or not SobelOperator::GenerateCoefficients() returns uses the old (ITK <= 5.4) coefficients for a 3D kernel. Enabled by default, unless `ITK_FUTURE_LEGACY_REMOVE` is ON.
N-Dekker
force-pushed
the
Make-SobelOperator-3D-consistent
branch
from
4c20b2c to
a20d509
Compare
4 participants
👉 Work in progress: The code still needs to have a clean-up. Moreover, it should still be confirmed that this is indeed the proper way to estimate those coefficients! An authoritative literature reference would be helpful, as well as more evidence that these are indeed the more commonly used coefficients!