Zernike moments#64
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Zernike-moment-based edge refinement algorithm and accompanying (visualization-oriented) test utilities, along with build-system updates to pull in required headers/libs.
Changes:
- Introduce
ZernikeEdgeDetectionAlgorithm(Zernike moments A₁₁/A₂₀) to refine edge points to sub-pixel accuracy. - Add test executables that load points from JSON and generate side-by-side visualizations (including a “thicker” variant).
- Update build tooling (CMake/Makefile) to fetch
nlohmann/jsonandstb_image_write, and adjust ignores/output artifacts.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/distance/edge.hpp |
Declares ZernikeEdgeDetectionAlgorithm and helper APIs. |
src/distance/edge.cpp |
Implements Zernike kernel generation, moment computation, and refinement loop. |
test/distance/edge-test/zernike-performance-test.cpp |
Adds JSON loading + visualization/perf-oriented gtests and image writing. |
test/distance/edge-test/zernike-single-asset-test.cpp |
Adds per-asset visualization tests across multiple window sizes. |
test/common/common.hpp |
Changes the example_earth1 asset path used broadly by tests. |
src/common/decimal.hpp |
Adds DECIMAL_EPSILON macro. |
CMakeLists.txt |
Fetches nlohmann_json and downloads stb_image_write.h for tests. |
Makefile |
Downloads nlohmann/json.hpp and stb_image_write.h, adds include paths for tests. |
.gitignore |
Ignores test/common/assets/zernike/. |
simple_edges.txt |
Adds a tracked edge-point dump output file. |
zernike_edges.txt |
Adds a tracked edge-point dump output file. |
Comments suppressed due to low confidence (1)
test/common/common.hpp:175
example_earth1now points totest/common/assets/example_earth3.JPG, but this constant is used across integration tests as the canonical “earth1” asset. This effectively changes the meaning of many existing tests and may break environments where onlyexample_earth1.pngis expected. Suggest keepingexample_earth1unchanged and introducing a separateexample_earth3asset constant (or updating all tests/assets consistently).
const ImageData example_earth1(
"test/common/assets/example_earth3.JPG",
85e-3,
20e-6,
{DegToRad(140), 0, 0},
{DECIMAL(10378137), 0, 0}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Nice work making the changes from last time! You're almost there. Hope these comments help. We'll talk soon at the meeting. |
nguy8tri
left a comment
nguy8tri
left a comment
There was a problem hiding this comment.
Good job on beginning the implementation of this complicated algorithm. However, your tests are failing, and normally I don't immediately provide feedback, but I think for your case it would be nice. Please fix my workflow before the next thing comes in. Most concerning is the lack of tests - We don't know if your algorithm does what it intends unless we test it.
Mostly some preliminary notes I've created. I'm also more interested in understanding the algorithm. Thanks!
|
Please also fill out the description |
nguy8tri
left a comment
nguy8tri
left a comment
There was a problem hiding this comment.
Good job addressing my comments, I have a few more changes I'd like to request!
| @@ -12,34 +12,41 @@ namespace found { | |||
| ///////// VECTOR CLASSES ////////// | |||
| /////////////////////////////////// | |||
|
|
|||
There was a problem hiding this comment.
This will disappear once Eigen is merged, but is necessary for now.
| /////////////////////////////////// | ||
|
|
||
| Vec2 Midpoint(const Vec2 &vec1, const Vec2 &vec2) { | ||
| Vec2<> Midpoint(const Vec2<> &vec1, const Vec2<> &vec2) { |
There was a problem hiding this comment.
Is it really valid to have empty brackets? Shouldn't there be a template? Is this C++'s version of a wildcard (like in Java)?
| decimal pixelWidth = DECIMAL(2.0) / maskSize_; | ||
| decimal offset = DECIMAL(1.0) - DECIMAL(1.0) / maskSize_; | ||
|
|
||
| constexpr int samplesPerPixel = 1028; |
There was a problem hiding this comment.
If its in a function I much prefer a macro
| decimal sum11 = 0; | ||
| decimal sum20 = 0; | ||
|
|
||
| for (int si = 0; si < samplesPerPixel; si++) { |
There was a problem hiding this comment.
Is there a way to do this inner double loop as a simple summation instead of a loop? Is there a way to figure out whether the condition of the pixel being in the unit disk applies? Notice how u and v increase with every iteration. You might be able to figure out what values will work using algebra or something.
| ////// Zernike Edge Detection Algorithm ////// | ||
|
|
||
| std::pair<std::unique_ptr<ComplexNumber[]>, std::unique_ptr<ComplexNumber[]>> | ||
| ZernikeEdgeDetectionAlgorithm::computeZernikeKernels() { |
There was a problem hiding this comment.
Have you considered using a set Zernike kernel? Accepting that as an optional parameter might be in your best interest as 1028*1028 is a lot of iterations.
| return {ComplexNumber{A11Real, A11Imag}, ComplexNumber{A20Val, 0}}; // A_20 is real-only | ||
| } | ||
|
|
||
| decimal ZernikeEdgeDetectionAlgorithm::extractEdgeAngle(const ComplexNumber &A11) { |
| return DECIMAL_ATAN2(A11.imag, A11.real); // Eq.(56) | ||
| } | ||
|
|
||
| decimal ZernikeEdgeDetectionAlgorithm::extractEdgeOffset(decimal A11Prime, decimal A20) { |
| return l; | ||
| } | ||
|
|
||
| Vec2<int> ZernikeEdgeDetectionAlgorithm::applyEdgeCorrection(const Vec2<int>& maskCenter, decimal l, decimal psi) { |
| * | ||
| * @return a pair of ComplexNumbers (A_11, A_20) | ||
| */ | ||
| std::pair<ComplexNumber, ComplexNumber> computeZernikeMoments( |
There was a problem hiding this comment.
This operation is similar, but not the same as a convolution. Might be helpful to add a note that this is the same as "applying" the kernel"
Description
Implementing the ZernikeEdgeDetectionAlgorithm, which ngests a pixel level edge detection algorithm to obtain a pixel level approximation of the edge of earth and then performs a sub-pixel edge correction using the moment based edge-orientation and offset. This leverages Zernike polynomials, which is a mathematical pattern defined in polar coordinates.
This algorithm is based on Christian (2017) "Accurate Planetary Limb Localization" (https://www.researchgate.net/publication/317742756_Accurate_Planetary_Limb_Localization_for_Image-Based_Spacecraft_Navigation)
Artifacts for PR #64 (DO NOT CHANGE)