docs: Update documentation - #388
Conversation
WalkthroughComprehensive documentation overhaul for the Img2Num library spanning user guides, API references across four language bindings (JavaScript/WASM, C++, C, Python), and operational guidance; paired with Python binding docstring enhancements and minor C binding refactoring. ChangesUser-Facing Documentation Foundation
API Reference Documentation for All Bindings
Python Binding Implementation and Internal Documentation
Build System, C Binding Refactoring, and Navigation Updates
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Ryan-Millard here I worked with codex to generate and update documentation. It actually looks pretty good but a few mistakes. Feel free to review and I will iteratively address issues |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs/api-reference.md`:
- Around line 40-54: The fenced ASCII pipeline block (the triple-backtick block
containing the diagram with nodes like "[Raster Image]", "[ Bilateral Filter]",
"[ K-Means Clustering]", "[ Contour Detection]", and "[ SVG Output]") is missing
a language identifier; update the opening fence from ``` to ```text so the code
fence has the `text` language identifier to satisfy MD040.
In `@docs/docs/c/docs/index.md`:
- Around line 11-23: The ordered list in the documentation uses mixed numbering
(the second item is "2.") and violates markdownlint MD029; update the ordered
list so every item uses the continued "1." style (i.e., change the "2. Copy the
header and shared library:" item to "1.") so both list entries ("Build the C++
library:" and "Copy the header and shared library:") use the normalized "1."
prefix.
In `@docs/docs/concepts.md`:
- Around line 27-30: The fenced code block containing the lines with "result =
weighted_average(input_pixels)" and the weight formula is missing a language
identifier; update the opening fence from ``` to ```text so the block has an
explicit language tag (e.g., change the fence that precedes the weighted_average
example to use ```text) to satisfy the MD040 rule.
In `@docs/docs/cpp/docs/index.md`:
- Around line 7-19: The ordered list numbering in the markdown uses mixed
prefixes (first item "1.", second item "2.") which triggers MD029; normalize the
list by using consistent "1." prefixes for each ordered item—update the list
entries "Build the library:" and "Link against `img2num`:" so both lines start
with "1." to satisfy markdownlint.
In `@docs/docs/installation.md`:
- Around line 29-38: The fenced code block inside the tip admonition (the :::tip
block that contains the import script with imageToUint8ClampedArray,
bilateralFilter, kmeans, findContours) must have a blank line before and after
the ``` fenced block to satisfy MD031; edit the tip block to insert an empty
line immediately above the opening ``` and immediately below the closing ```,
preserving the existing content and fence markers.
- Around line 63-65: Add a language identifier to the fenced code block that
currently contains "bindings/c/include/cimg2num.h" (e.g., change the opening
fence from ``` to ```text or ```bash) so the Markdown linter rule MD040 is
satisfied; locate the fenced block that exactly contains that path and update
the opening backticks to include the chosen language token.
In `@docs/docs/js/api-reference.md`:
- Around line 51-53: The documentation for imageToUint8ClampedArray currently
conflicts between a tuple and an object return shape; pick one canonical shape
(prefer the object form { pixels: Uint8ClampedArray, width: number, height:
number } or the tuple [pixels, width, height]) and update both occurrences to
match; ensure the signature text and the "Returns:" line both describe the same
return type and reference the function name imageToUint8ClampedArray so readers
and any generated API docs are consistent.
- Line 9: The docs state "All functions are exported from the img2num package"
and claim they are async, but several function return examples are written as
plain objects; update those return type examples to Promise-wrapped types (e.g.,
change FooResponse to Promise<FooResponse>) wherever a function's doc/example
currently shows a plain object return so the API page consistently reflects
async behavior for all exported functions from img2num; locate the return
sections for each exported function in this file and change their documented
return signatures/examples to Promise<...> to match the actual async functions.
In `@docs/docs/js/docs/index.md`:
- Around line 7-29: The code fence containing the ASCII architecture diagram is
missing a language tag; change the opening fence from ``` to ```text so the
block is treated as plain text (e.g., update the diagram block that lists
safeWasmWrappers.js, wasmClient.js, wasmWorker.js, and img2num_core.wasm to
start with ```text).
In `@docs/docs/performance.md`:
- Line 38: The code destructures a non-existent pixels property from
ctx.getImageData; change the destructuring to use the actual ImageData property
name `data` instead of `pixels` (e.g., replace `const { pixels, width, height }
= ctx.getImageData(…)` with `const { data, width, height } =
ctx.getImageData(…)`) so subsequent code references the correct buffer returned
by getImageData().
In `@docs/docs/python/api-reference.md`:
- Around line 9-11: The docs contradict whether width/height are injected from
NumPy arrays or required keyword parameters; pick one contract and make all
signatures, parameter tables, and examples consistent: either (A) document that
img2num functions (e.g., image_to_svg) accept a NumPy array and automatically
derive width/height (remove width,height from the function signatures and mark
them as derived in the parameter descriptions and examples), or (B) document
that width and height are explicit required keyword-only args (keep them in the
signatures and update the top-level statement to stop claiming automatic
injection); apply the chosen approach to image_to_svg and the other listed
function docs so signatures, parameter descriptions, and examples match.
- Line 60: The docs use the removed alias NDArray[np.int]; update both
occurrences (the Returns tuple shown as NDArray[np.uint8], NDArray[np.int] and
the other occurrence) to a supported explicit integer type—replace
NDArray[np.int] with NDArray[np.int_] (or NDArray[np.int64]/NDArray[np.int32] if
you need fixed width) so the documentation uses a valid NumPy type instead of
the deprecated np.int.
In `@docs/docs/troubleshooting.md`:
- Line 32: The sentence about tuning sigma_spatial is self-contradictory; update
the text to be directional by symptom: state that if the result is
over-smoothed, decrease sigma_spatial, and if the result is too noisy or
contains speckle artifacts, increase sigma_spatial. Edit the sentence
referencing `sigma_spatial` so it clearly maps each symptom to the proper action
(e.g., "If the image is over-smoothed, decrease `sigma_spatial`; if the image is
too noisy, increase `sigma_spatial`.").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b1b7aa9-6846-4f80-a308-e36d5784f6c4
📒 Files selected for processing (16)
core/include/img2num.hdocs/docs/api-reference.mddocs/docs/c/api-reference.mddocs/docs/c/docs/index.mddocs/docs/concepts.mddocs/docs/cpp/api-reference.mddocs/docs/cpp/docs/index.mddocs/docs/getting-started.mddocs/docs/index.mddocs/docs/installation.mddocs/docs/js/api-reference.mddocs/docs/js/docs/index.mddocs/docs/performance.mddocs/docs/python/api-reference.mddocs/docs/python/index.mddocs/docs/troubleshooting.md
💤 Files with no reviewable changes (1)
- core/include/img2num.h
📜 Review details
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build C/C++ / Build C & C++
- GitHub Check: Build C/C++ / Build Python
- GitHub Check: Build Documentation Site / Build Docusaurus Site
- GitHub Check: Build C/C++ / Build WASM (bindings/js)
- GitHub Check: Lint & Validate Code
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
📄 CodeRabbit inference engine (.editorconfig)
**/*.md: Do not trim trailing whitespace in Markdown files
Use 2-space indentation for Markdown files
Do not enforce maximum line length for Markdown files
Files:
docs/docs/python/index.mddocs/docs/troubleshooting.mddocs/docs/installation.mddocs/docs/index.mddocs/docs/getting-started.mddocs/docs/api-reference.mddocs/docs/python/api-reference.mddocs/docs/js/docs/index.mddocs/docs/js/api-reference.mddocs/docs/concepts.mddocs/docs/cpp/docs/index.mddocs/docs/c/docs/index.mddocs/docs/performance.mddocs/docs/cpp/api-reference.mddocs/docs/c/api-reference.md
docs/docs/**
⚙️ CodeRabbit configuration file
docs/docs/**: All documentation must follow Docusaurus conventions (docs/docs/ folder structure).
Do NOT suggest creating strangely-named markdown files at the repository root
(e.g. COMPREHENSIVE_TEST_REPORT.md, DELIVERABLES.md, SUMMARY.md).
New docs belong either as a dedicated Docusaurus category or integrated into an
existing category such as project-scripts. Verify valid frontmatter
(title, sidebar_label, etc.) and correct sidebar wiring.
Files:
docs/docs/python/index.mddocs/docs/troubleshooting.mddocs/docs/installation.mddocs/docs/index.mddocs/docs/getting-started.mddocs/docs/api-reference.mddocs/docs/python/api-reference.mddocs/docs/js/docs/index.mddocs/docs/js/api-reference.mddocs/docs/concepts.mddocs/docs/cpp/docs/index.mddocs/docs/c/docs/index.mddocs/docs/performance.mddocs/docs/cpp/api-reference.mddocs/docs/c/api-reference.md
🧠 Learnings (2)
📚 Learning: 2026-01-04T15:34:04.654Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 146
File: docs/docs/reference/react/components/ThemeSwitch/tests.md:123-124
Timestamp: 2026-01-04T15:34:04.654Z
Learning: In Docusaurus documentation, relative directory links (e.g., ../ or ../../../hooks/useTheme) resolve to index.md within the target directory. Do not require or force linking to index.md explicitly; such relative links remain valid and should not be changed when reviewing docs under the docs directory.
Applied to files:
docs/docs/python/index.mddocs/docs/troubleshooting.mddocs/docs/installation.mddocs/docs/index.mddocs/docs/getting-started.mddocs/docs/api-reference.mddocs/docs/python/api-reference.mddocs/docs/js/docs/index.mddocs/docs/js/api-reference.mddocs/docs/concepts.mddocs/docs/cpp/docs/index.mddocs/docs/c/docs/index.mddocs/docs/performance.mddocs/docs/cpp/api-reference.mddocs/docs/c/api-reference.md
📚 Learning: 2026-02-22T15:20:15.304Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 263
File: docs/docs/internal/core/api-reference.md:13-13
Timestamp: 2026-02-22T15:20:15.304Z
Learning: In Docusaurus projects, Markdown files (*.md, *.mdx) may contain anchor tags with onClick={(e) => { e.preventDefault(); window.location.href = '...'; }} to force a full page navigation, bypassing React SPA routing for static/external paths. Treat this as intentional and do not flag it as redundant in docs contexts. Apply this guidance broadly to Markdown docs across the repository (not just the single file) and focus reviews on more impactful patterns such as accessibility, SEO, and navigation consistency.
Applied to files:
docs/docs/python/index.mddocs/docs/troubleshooting.mddocs/docs/installation.mddocs/docs/index.mddocs/docs/getting-started.mddocs/docs/api-reference.mddocs/docs/python/api-reference.mddocs/docs/js/docs/index.mddocs/docs/js/api-reference.mddocs/docs/concepts.mddocs/docs/cpp/docs/index.mddocs/docs/c/docs/index.mddocs/docs/performance.mddocs/docs/cpp/api-reference.mddocs/docs/c/api-reference.md
🪛 LanguageTool
docs/docs/troubleshooting.md
[style] ~48-~48: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...The WASM heap is fixed at compile time. Very large images may exceed it. Consider: - Down...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.22.1)
docs/docs/installation.md
[warning] 37-37: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/docs/api-reference.md
[warning] 40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/docs/js/docs/index.md
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/docs/concepts.md
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/docs/cpp/docs/index.md
[warning] 14-14: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
docs/docs/c/docs/index.md
[warning] 18-18: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
🔇 Additional comments (4)
docs/docs/index.md (1)
9-35: LGTM!docs/docs/getting-started.md (1)
9-87: LGTM!docs/docs/performance.md (1)
1-37: LGTM!Also applies to: 39-51
docs/docs/troubleshooting.md (1)
1-31: LGTM!Also applies to: 33-56
| 1. Build the C++ library: | ||
|
|
||
| ```bash | ||
| cmake -B build . | ||
| cmake --build build | ||
| ``` | ||
|
|
||
| 2. Copy the header and shared library: | ||
|
|
||
| ```bash | ||
| cp bindings/c/include/cimg2num.h /your/project/dir | ||
| cp build/libimg2num.so /your/project/dir # .dylib on macOS, .dll on Windows | ||
| ``` |
There was a problem hiding this comment.
Normalize ordered list numbering for markdownlint.
Line 18 uses 2. while this doc style/lint expects 1. prefixes for ordered lists (MD029).
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 18-18: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/c/docs/index.md` around lines 11 - 23, The ordered list in the
documentation uses mixed numbering (the second item is "2.") and violates
markdownlint MD029; update the ordered list so every item uses the continued
"1." style (i.e., change the "2. Copy the header and shared library:" item to
"1.") so both list entries ("Build the C++ library:" and "Copy the header and
shared library:") use the normalized "1." prefix.
| ``` | ||
| result = weighted_average(input_pixels) | ||
| weight = exp(-dist² / 2σ²_spatial) × exp(-Δintensity² / 2σ²_range) | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the fenced code block.
The block starting at Line 27 is missing a language tag (MD040). text is sufficient here.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/concepts.md` around lines 27 - 30, The fenced code block containing
the lines with "result = weighted_average(input_pixels)" and the weight formula
is missing a language identifier; update the opening fence from ``` to ```text
so the block has an explicit language tag (e.g., change the fence that precedes
the weighted_average example to use ```text) to satisfy the MD040 rule.
| 1. Build the library: | ||
|
|
||
| > You don't need an issue to work on this - just mention it in your PR body.🦔 | ||
| ```bash | ||
| cmake -B build . | ||
| cmake --build build | ||
| ``` | ||
|
|
||
| 2. Link against `img2num`: | ||
|
|
||
| ```cmake | ||
| find_package(img2num REQUIRED) | ||
| target_link_libraries(myapp PRIVATE img2num) | ||
| ``` |
There was a problem hiding this comment.
Normalize ordered list numbering for markdownlint.
Line 14 uses 2. but the configured markdownlint rule expects 1. list prefixes (MD029).
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 14-14: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/cpp/docs/index.md` around lines 7 - 19, The ordered list numbering
in the markdown uses mixed prefixes (first item "1.", second item "2.") which
triggers MD029; normalize the list by using consistent "1." prefixes for each
ordered item—update the list entries "Build the library:" and "Link against
`img2num`:" so both lines start with "1." to satisfy markdownlint.
| canvas.height = 720; | ||
| const ctx = canvas.getContext("2d"); | ||
| ctx.drawImage(image, 0, 0, 720, 720); | ||
| const { pixels, width, height } = ctx.getImageData(0, 0, 720, 720); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What object and properties does CanvasRenderingContext2D.getImageData() return (spec/MDN), and is there a pixels property?
💡 Result:
The CanvasRenderingContext2D.getImageData method returns an ImageData object [1][2]. There is no "pixels" property on the ImageData object [3]. The ImageData object contains the following instance properties [3]: - data: A Uint8ClampedArray or Float16Array (depending on the pixel format) that contains the image data as a one-dimensional array in RGBA order [3][4]. - width: An unsigned long representing the actual width of the image data in pixels [3]. - height: An unsigned long representing the actual height of the image data in pixels [3]. - colorSpace: A string indicating the color space of the image data [3]. - pixelFormat: A string indicating the format of the image data (e.g., "rgba-unorm8" or "rgba-float16") [3]. The pixel data itself is accessed via the.data property, not a "pixels" property [3].
Citations:
- 1: https://wiki.developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/getImageData
- 2: https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/getImageData
- 3: https://developer.cdn.mozilla.net/en-US/docs/Web/API/ImageData
- 4: https://developer.mozilla.org/en-US/docs/Web/API/ImageData/data
🏁 Script executed:
# Check if the file exists and read its content
if [ -f "docs/docs/performance.md" ]; then
echo "=== File exists, reading content (first 50 lines) ==="
head -50 "docs/docs/performance.md" | cat -n
else
echo "File not found. Searching for performance.md in docs folder:"
find docs -name "performance.md" 2>/dev/null
fiRepository: Ryan-Millard/Img2Num
Length of output: 1861
🏁 Script executed:
# Also check if there are other .md files in docs/docs to understand structure
echo "=== Docusaurus docs structure ==="
ls -la docs/docs/ 2>/dev/null | head -20Repository: Ryan-Millard/Img2Num
Length of output: 1007
Use data from ImageData, not pixels.
Line 38 destructures a non-existent pixels property. getImageData() returns { data, width, height }; pixels is undefined.
Suggested doc fix
-const { pixels, width, height } = ctx.getImageData(0, 0, 720, 720);
+const { data: pixels, width, height } = ctx.getImageData(0, 0, 720, 720);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { pixels, width, height } = ctx.getImageData(0, 0, 720, 720); | |
| const { data: pixels, width, height } = ctx.getImageData(0, 0, 720, 720); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/performance.md` at line 38, The code destructures a non-existent
pixels property from ctx.getImageData; change the destructuring to use the
actual ImageData property name `data` instead of `pixels` (e.g., replace `const
{ pixels, width, height } = ctx.getImageData(…)` with `const { data, width,
height } = ctx.getImageData(…)`) so subsequent code references the correct
buffer returned by getImageData().
| All functions are exposed via the `img2num` Python package. They accept NumPy arrays and automatically inject `width`/`height` from the array shape. | ||
|
|
||
| ## `image_to_svg(image, *, width, height, config=None)` |
There was a problem hiding this comment.
Resolve the width/height contract contradiction.
Line 9 says width/height are injected automatically, but the documented signatures make them explicit required keyword-only parameters. Please choose one contract and align signatures, parameter tables, and examples accordingly.
Also applies to: 20-23, 32-33, 47-48, 62-63
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/python/api-reference.md` around lines 9 - 11, The docs contradict
whether width/height are injected from NumPy arrays or required keyword
parameters; pick one contract and make all signatures, parameter tables, and
examples consistent: either (A) document that img2num functions (e.g.,
image_to_svg) accept a NumPy array and automatically derive width/height (remove
width,height from the function signatures and mark them as derived in the
parameter descriptions and examples), or (B) document that width and height are
explicit required keyword-only args (keep them in the signatures and update the
top-level statement to stop claiming automatic injection); apply the chosen
approach to image_to_svg and the other listed function docs so signatures,
parameter descriptions, and examples match.
| | `max_iter` | `int` | — | | ||
| | `color_space` | `int` | — | | ||
|
|
||
| **Returns:** `(NDArray[np.uint8], NDArray[np.int])` — `(clustered_data, labels)` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is np.int deprecated in current NumPy, and what is the recommended replacement for type annotations like NDArray[np.int]?
💡 Result:
Yes, np.int was deprecated in NumPy 1.20 and subsequently removed in NumPy 1.24 [1][2]. Attempting to use it in current versions will raise an AttributeError [3][2]. For type annotations, the recommended replacement for the deprecated np.int is to use np.int_ or an explicit fixed-precision type like np.int64 or np.int32, depending on your requirements [1][2]. When using NDArray[...], you should update your annotations to use these valid NumPy scalar types [4][5]. For example: Replace: NDArray[np.int] With: NDArray[np.int_] # For platform-dependent integer precision [1][2] OR NDArray[np.int64] # For explicit 64-bit integer precision [1][2] If your intent is simply to annotate a generic integer type and you do not require specific NumPy scalar behavior, you may also use Python's built-in int type, though NDArray typically expects a NumPy scalar type parameter for the dtype [1][2]. The np.int_ alias remains the closest equivalent to the old behavior, as it corresponds to the default system integer type [1][2].
Citations:
- 1: https://numpy.org/devdocs/release/1.20.0-notes.html
- 2: https://stackoverflow.com/questions/74946845/attributeerror-module-numpy-has-no-attribute-int
- 3:
np.intwas a deprecated alias for the builtinint. scikit-optimize/scikit-optimize#1171 - 4: https://numpy.org/doc/stable/reference/typing.html
- 5: https://numpy.org/devdocs/reference/typing.html
🏁 Script executed:
cat -n docs/docs/python/api-reference.md | sed -n '55,75p'Repository: Ryan-Millard/Img2Num
Length of output: 655
🏁 Script executed:
fd -e txt -e cfg -e toml -e py -e lock 2>/dev/null | grep -E "(requirements|setup|pyproject|poetry)" | head -20Repository: Ryan-Millard/Img2Num
Length of output: 145
🏁 Script executed:
cat -n pyproject.toml | head -50Repository: Ryan-Millard/Img2Num
Length of output: 1415
🏁 Script executed:
cat packages/py/pyproject.tomlRepository: Ryan-Millard/Img2Num
Length of output: 688
Replace deprecated np.int with explicit type in documentation.
np.int was removed in NumPy 1.24. Use np.int_ for platform-dependent integer precision, or explicit np.int64/np.int32 for fixed precision in return and parameter type annotations.
Lines: 60, 70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/python/api-reference.md` at line 60, The docs use the removed alias
NDArray[np.int]; update both occurrences (the Returns tuple shown as
NDArray[np.uint8], NDArray[np.int] and the other occurrence) to a supported
explicit integer type—replace NDArray[np.int] with NDArray[np.int_] (or
NDArray[np.int64]/NDArray[np.int32] if you need fixed width) so the
documentation uses a valid NumPy type instead of the deprecated np.int.
|
|
||
| ## Blurry or Over-Smoothed Output | ||
|
|
||
| Increase `sigma_spatial` or decrease it depending on the image. If it's over-smoothed, reduce it. |
There was a problem hiding this comment.
Clarify the tuning direction for sigma_spatial.
This sentence is self-contradictory (“increase or decrease”). Make it directional by symptom.
Suggested wording
-Increase `sigma_spatial` or decrease it depending on the image. If it's over-smoothed, reduce it.
+If output looks noisy or jagged, increase `sigma_spatial`. If output is over-smoothed or blurry, decrease `sigma_spatial`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Increase `sigma_spatial` or decrease it depending on the image. If it's over-smoothed, reduce it. | |
| If output looks noisy or jagged, increase `sigma_spatial`. If output is over-smoothed or blurry, decrease `sigma_spatial`. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/troubleshooting.md` at line 32, The sentence about tuning
sigma_spatial is self-contradictory; update the text to be directional by
symptom: state that if the result is over-smoothed, decrease sigma_spatial, and
if the result is too noisy or contains speckle artifacts, increase
sigma_spatial. Edit the sentence referencing `sigma_spatial` so it clearly maps
each symptom to the proper action (e.g., "If the image is over-smoothed,
decrease `sigma_spatial`; if the image is too noisy, increase
`sigma_spatial`.").
|
Hi @Krasner. I don't have time to debug this right now, but it's possible that the version of Docusaurus we're using isn't compatible with the syntax you're using. I think it was v3.10 that broke the old syntax. |
No worries. There are some doxygen files missing as well. This is a good experiment working with llm tools to see how well they can understand code and instructions. So far it's very slow.... (but free and local) |
CodeRabbit can do it quite well since it has our repository's context, but you have to tell it which files and so on. At the end of the day, though, AI still needs human intervention because it isn't amazing. |
Krasner
changed the title
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/c/src/cimg2num.cpp`:
- Around line 11-15: The return uses C++20 designated-initializer syntax which
breaks C++17 builds; update the construction of img2num::ImageToSvgConfig in the
to_cpp/to_c conversion functions to use C++17-compatible aggregate
initialization or explicit member assignment instead of `.member{...}` syntax —
e.g. replace `return {.bilateral_filter{...}, .kmeans{...}, .min_cluster_area,
.color_space}` with a plain braced aggregate `return {{...}, {...}, ..., ...}`
or build a local ImageToSvgConfig and assign bilateral_filter, kmeans,
min_cluster_area, color_space members before returning.
In `@bindings/py/doxygen/home_page.dox`:
- Line 70: Update the example call to bilateral_filter in home_page.dox to
include the required color_space argument used by the binding; locate the usage
of _img2num.bilateral_filter in the doc and add the color_space parameter
(matching the expected type/value used by the binding in img2num_pybind.cpp for
bilateral_filter) so the example signature matches the actual function
(including color_space, sigma_spatial, sigma_range).
- Line 67: The example call to _img2num.kmeans is missing the required
color_space parameter; update the usage to pass the appropriate color space
(e.g., "rgb" or the expected enum/string) as the color_space argument when
calling _img2num.kmeans so it matches the binding signature defined in
img2num_pybind.cpp and prevents the runtime error.
In `@bindings/py/src/img2num_pybind.cpp`:
- Around line 200-201: Replace the C-style casts on the pybind11::array_t
construction for out_labels and use C++ style casts: change {(ssize_t)height,
(ssize_t)width} to use static_cast<ssize_t>(height) and
static_cast<ssize_t>(width) so the pybind11::array_t<int32_t,
pybind11::array::c_style> out_labels initialization follows the project’s Google
C++ style and matches the static_cast usage in the surrounding code.
In `@docs/docs/contributing/index.md`:
- Line 13: Replace the hardcoded .md file URL in the "Code of Conduct" link in
docs/docs/contributing/index.md (the link text "Code of Conduct" currently
pointing to /docs/contributing/code-of-conduct.md) with the Docusaurus doc route
(remove the .md extension), e.g. /docs/contributing/code-of-conduct or
/docs/contributing/code-of-conduct/ so the site router resolves it correctly;
update the link target and confirm the referenced document has valid frontmatter
and is wired into the sidebar.
In `@docs/docs/index.md`:
- Around line 29-34: The table in docs/docs/index.md contains broken links for
the API references (the link text tokens like [JS API Reference], [C++ API
Reference], [C API Reference], [Python API Reference]) that currently point to
paths such as /docs/next/js/js-api-reference, /docs/next/cpp/cpp-api-reference,
etc.; update each anchor to the canonical Docusaurus routes: change the JS link
to /docs/next/js/api-reference, C++ to /docs/next/cpp/api-reference, C to
/docs/next/c/api-reference, and Python to /docs/next/python/api-reference so the
table points to the correct docs locations.
In `@docs/docs/internal/bindings/py/index.md`:
- Around line 50-73: The docs example shows a high-level API (from img2num
import gaussian_blur_fft, invert_image, threshold_image, image_to_svg) but the
actual bindings in img2num_pybind.cpp expose low-level functions that require
explicit width and height (e.g., _img2num.gaussian_blur_fft(image, width,
height, sigma)), causing a mismatch; update the example to call the real binding
signatures (use _img2num.* functions and pass width and height to
gaussian_blur_fft, invert_image, threshold_image, image_to_svg) or explicitly
state this snippet is for a higher-level wrapper and link to that wrapper
implementation, mentioning the binding file _img2num and img2num_pybind.cpp so
maintainers can reconcile the two.
In `@docs/docs/js/index.md`:
- Line 127: Update the broken API link in docs/docs/js/index.md so it points to
the actual page route for the JavaScript API reference (either the site root
route /docs/js/api-reference/ or the relative path ./api-reference.md depending
on your Docusaurus routing), replacing the incorrect target
/docs/next/js/js-api-reference/; verify the target matches the doc file
docs/docs/js/api-reference.md and that the frontmatter/sidebar config exposes
the route so the link resolves correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c596e4da-ff60-481b-9e8b-3a987bb94175
📒 Files selected for processing (25)
README.mdbindings/c/include/cimg2num.hbindings/c/src/cimg2num.cppbindings/js/doxygen/home_page.doxbindings/py/Doxyfile.internalbindings/py/doxygen/home_page.doxbindings/py/src/img2num_pybind.cppdocs/docs/api-reference.mddocs/docs/concepts.mddocs/docs/contributing/index.mddocs/docs/cpp/api-reference.mddocs/docs/cpp/docs/index.mddocs/docs/getting-started.mddocs/docs/index.mddocs/docs/installation.mddocs/docs/internal/bindings/py/api-reference.mddocs/docs/internal/bindings/py/index.mddocs/docs/internal/index.mddocs/docs/js/api-reference.mddocs/docs/js/docs/index.mddocs/docs/js/index.mddocs/docs/performance.mddocs/docs/python/api-reference.mddocs/docs/python/index.mddocs/scripts/doxygen.js
💤 Files with no reviewable changes (1)
- bindings/c/include/cimg2num.h
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.md
📄 CodeRabbit inference engine (.editorconfig)
**/*.md: Do not trim trailing whitespace in Markdown files
Use 2-space indentation for Markdown files
Do not enforce maximum line length for Markdown files
Files:
docs/docs/contributing/index.mddocs/docs/js/docs/index.mddocs/docs/internal/bindings/py/index.mdREADME.mddocs/docs/python/index.mddocs/docs/js/api-reference.mddocs/docs/internal/bindings/py/api-reference.mddocs/docs/python/api-reference.mddocs/docs/installation.mddocs/docs/internal/index.mddocs/docs/js/index.mddocs/docs/getting-started.mddocs/docs/concepts.mddocs/docs/api-reference.mddocs/docs/cpp/docs/index.mddocs/docs/index.mddocs/docs/performance.mddocs/docs/cpp/api-reference.md
docs/docs/**
⚙️ CodeRabbit configuration file
docs/docs/**: All documentation must follow Docusaurus conventions (docs/docs/ folder structure).
Do NOT suggest creating strangely-named markdown files at the repository root
(e.g. COMPREHENSIVE_TEST_REPORT.md, DELIVERABLES.md, SUMMARY.md).
New docs belong either as a dedicated Docusaurus category or integrated into an
existing category such as project-scripts. Verify valid frontmatter
(title, sidebar_label, etc.) and correct sidebar wiring.
Files:
docs/docs/contributing/index.mddocs/docs/js/docs/index.mddocs/docs/internal/bindings/py/index.mddocs/docs/python/index.mddocs/docs/js/api-reference.mddocs/docs/internal/bindings/py/api-reference.mddocs/docs/python/api-reference.mddocs/docs/installation.mddocs/docs/internal/index.mddocs/docs/js/index.mddocs/docs/getting-started.mddocs/docs/concepts.mddocs/docs/api-reference.mddocs/docs/cpp/docs/index.mddocs/docs/index.mddocs/docs/performance.mddocs/docs/cpp/api-reference.md
bindings/**
⚙️ CodeRabbit configuration file
bindings/**: Language bindings for the Img2Num library. Ensure the public API surface matches
the core C/C++ headers and that error propagation is handled correctly for each
binding language. Also ensure the relevant docstrings are present to enable
auto-generation of documentation.
Files:
bindings/py/Doxyfile.internalbindings/py/doxygen/home_page.doxbindings/js/doxygen/home_page.doxbindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{js,ts,jsx,tsx}: Use 2-space indentation for JavaScript and TypeScript files
Maintain 200 character maximum line length for JavaScript/TypeScript files
Files:
docs/scripts/doxygen.js
docs/**/*.{js,jsx,ts,tsx,mdx}
⚙️ CodeRabbit configuration file
docs/**/*.{js,jsx,ts,tsx,mdx}: This is the Docusaurus site source. Check for MDX compatibility, correct
plugin usage, and valid imports. Review docusaurus.config.js changes carefully
for broken routing or plugin config regressions.
Files:
docs/scripts/doxygen.js
**/*.{c,cc,cpp,cxx,h,hpp,hxx}
📄 CodeRabbit inference engine (.clang-format)
**/*.{c,cc,cpp,cxx,h,hpp,hxx}: Follow Google style guide for C/C++ code formatting
Use 4 spaces for indentation
Maintain a column limit of 100 characters per line
Do not allow short functions on a single line
Files:
bindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
**/*.{hpp,cpp,c,h}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{hpp,cpp,c,h}: Use 4-space indentation for C/C++ files
Maintain 120 character maximum line length for C/C++ files
Files:
bindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
**/*.{cpp,c,h,hpp}
⚙️ CodeRabbit configuration file
**/*.{cpp,c,h,hpp}: - For any C/C++ code outside core/ (e.g. bindings, example-apps), apply the same
memory-safety and style standards as the core. Formatting via./img2num format-cpp.
- Doxygen docstrings are required.
Files:
bindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: Ryan-Millard/Img2Num
Timestamp: 2026-05-24T14:06:11.424Z
Learning: Use descriptive titles when reporting issues
Learnt from: CR
Repo: Ryan-Millard/Img2Num
Timestamp: 2026-05-24T14:06:11.424Z
Learning: Include steps to reproduce, expected behavior, and actual behavior when reporting bugs
Learnt from: CR
Repo: Ryan-Millard/Img2Num
Timestamp: 2026-05-24T14:06:11.424Z
Learning: Attach screenshots or logs to issue reports when applicable
Learnt from: CR
Repo: Ryan-Millard/Img2Num
Timestamp: 2026-05-24T14:06:11.424Z
Learning: Specify environment details (OS, Node.js version, browser) when reporting issues
Learnt from: CR
Repo: Ryan-Millard/Img2Num
Timestamp: 2026-05-24T14:06:11.424Z
Learning: Review and adhere to the Code of Conduct to foster an open and welcoming environment
📚 Learning: 2026-01-04T15:34:04.654Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 146
File: docs/docs/reference/react/components/ThemeSwitch/tests.md:123-124
Timestamp: 2026-01-04T15:34:04.654Z
Learning: In Docusaurus documentation, relative directory links (e.g., ../ or ../../../hooks/useTheme) resolve to index.md within the target directory. Do not require or force linking to index.md explicitly; such relative links remain valid and should not be changed when reviewing docs under the docs directory.
Applied to files:
docs/docs/contributing/index.mddocs/docs/js/docs/index.mddocs/docs/internal/bindings/py/index.mddocs/docs/python/index.mddocs/docs/js/api-reference.mddocs/docs/internal/bindings/py/api-reference.mddocs/docs/python/api-reference.mddocs/docs/installation.mddocs/docs/internal/index.mddocs/docs/js/index.mddocs/docs/getting-started.mddocs/docs/concepts.mddocs/docs/api-reference.mddocs/docs/cpp/docs/index.mddocs/docs/index.mddocs/docs/performance.mddocs/docs/cpp/api-reference.md
📚 Learning: 2026-02-22T15:20:15.304Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 263
File: docs/docs/internal/core/api-reference.md:13-13
Timestamp: 2026-02-22T15:20:15.304Z
Learning: In Docusaurus projects, Markdown files (*.md, *.mdx) may contain anchor tags with onClick={(e) => { e.preventDefault(); window.location.href = '...'; }} to force a full page navigation, bypassing React SPA routing for static/external paths. Treat this as intentional and do not flag it as redundant in docs contexts. Apply this guidance broadly to Markdown docs across the repository (not just the single file) and focus reviews on more impactful patterns such as accessibility, SEO, and navigation consistency.
Applied to files:
docs/docs/contributing/index.mddocs/docs/js/docs/index.mddocs/docs/internal/bindings/py/index.mdREADME.mddocs/docs/python/index.mddocs/docs/js/api-reference.mddocs/docs/internal/bindings/py/api-reference.mddocs/docs/python/api-reference.mddocs/docs/installation.mddocs/docs/internal/index.mddocs/docs/js/index.mddocs/docs/getting-started.mddocs/docs/concepts.mddocs/docs/api-reference.mddocs/docs/cpp/docs/index.mddocs/docs/index.mddocs/docs/performance.mddocs/docs/cpp/api-reference.md
📚 Learning: 2025-12-31T17:46:54.476Z
Learnt from: fransafu
Repo: Ryan-Millard/Img2Num PR: 176
File: src/wasm/modules/image/src/bilateral_filter.cpp:98-101
Timestamp: 2025-12-31T17:46:54.476Z
Learning: In bilateral_filter.cpp (src/wasm/modules/image/src/bilateral_filter.cpp), rely on the fact that the center pixel contributes a weight of exactly 1.0 to both spatial and range components (exp(0) = 1) so the normalization sum cannot be zero. This implies you do not need explicit guards against division by zero for normalization in this implementation.
Applied to files:
bindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-06T04:56:57.269Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/kmeans.cpp:128-134
Timestamp: 2026-01-06T04:56:57.269Z
Learning: In C++ sources compiled for WASM with Emscripten/Clang, designated initializers (e.g., RGBXY{.r = ..., .g = ...}) are allowed as a C++17 extension. When reviewing code that relies on designated initializers, verify that the target toolchain enables CXX_STANDARD 17 or higher and that the build system (CMake/emsdk) uses Emscripten with a compatible clang. If not, avoid such initializers or provide portable alternatives.
Applied to files:
bindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-06T21:06:24.476Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 191
File: src/wasm/modules/image/src/cielab.cpp:43-57
Timestamp: 2026-01-06T21:06:24.476Z
Learning: In the Img2Num project, prefer recommending and using the provided docker/script wrappers (e.g., ./img2num format-wasm, ./img2num clean-wasm) instead of invoking local tools directly (e.g., clang-format -i). This reduces dependency requirements for users and ensures consistent tooling across environments. Apply this guidance to C++ source files under the project when reviewing changes.
Applied to files:
bindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-01-19T00:02:34.957Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 234
File: src/wasm/modules/image/src/kmeans.cpp:79-81
Timestamp: 2026-01-19T00:02:34.957Z
Learning: In C++ multithreading contexts, prefer unsigned types for thread-count-like parameters (e.g., n_threads) to prevent negative values. Validate that the value is at least 1 before any division to avoid divide-by-zero at runtime. At the start of functions handling such values, create a safe count like: const unsigned int thread_count{std::max(1u, n_threads)}; This ensures non-negative, non-zero usage for divisions and related arithmetic. Apply this pattern to similar parameters across C++ modules, not just the specific file.
Applied to files:
bindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-02-25T21:24:19.036Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 272
File: core/src/internal/bilateral_filter_gpu.cpp:171-185
Timestamp: 2026-02-25T21:24:19.036Z
Learning: Adopt brace initialization (e.g., Type var{}) over copy initialization (e.g., Type var = {}) for zero-initialization in C++ across the repository. This should apply to variables, arrays, and structs in most C++ files (e.g., core/src/internal/bilateral_filter_gpu.cpp). It improves safety by avoiding narrowing conversions and makes initialization intent explicit.
Applied to files:
bindings/c/src/cimg2num.cppbindings/py/src/img2num_pybind.cpp
📚 Learning: 2026-04-27T15:40:33.329Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 333
File: bindings/c/src/cimg2num.cpp:0-0
Timestamp: 2026-04-27T15:40:33.329Z
Learning: In the Ryan-Millard/Img2Num C bindings (e.g., `bindings/c/src/cimg2num.cpp`) that expose `extern "C"` functions, do not allow C++ exceptions (including `std::bad_alloc` and exceptions thrown from lambdas used internally) to propagate out of these C binding functions. Use the C-API failure idiom instead: catch exceptions internally as needed, clear/record error state via the existing error mechanism, and return `nullptr` (or the documented sentinel return value) on failure so the C caller can detect errors by checking the return value.
Applied to files:
bindings/c/src/cimg2num.cpp
🪛 Cppcheck (2.20.0)
bindings/py/src/img2num_pybind.cpp
[style] 11-11: The function 'PYBIND11_MODULE' is never used.
(unusedFunction)
🔇 Additional comments (21)
docs/docs/concepts.md (1)
15-16: LGTM!Also applies to: 18-18, 39-39
docs/docs/installation.md (1)
34-34: LGTM!Also applies to: 37-37
docs/docs/getting-started.md (1)
85-87: LGTM!docs/docs/performance.md (1)
16-16: LGTM!Also applies to: 21-27
docs/docs/js/api-reference.md (2)
9-9: Align return types with the async contract on this page.The page says all exported functions are async, but multiple sections still document plain object return types instead of Promise-wrapped returns.
Also applies to: 24-24, 41-41, 47-47
51-53: Use one canonical return shape forimageToUint8ClampedArray.This section still mixes tuple and object return formats; keep one representation consistently in both the description and Returns line.
docs/docs/python/api-reference.md (2)
9-11: Resolve thewidth/heightcontract inconsistency across signatures, tables, and examples.The page still states automatic shape-derived dimensions while documenting/using explicit keyword-only
widthandheight. Pick one contract and apply it consistently.Also applies to: 20-23, 29-29, 32-33, 47-48, 62-63
60-60: Replace deprecated NumPynp.intin the documented type hints.
np.intis deprecated/removed in modern NumPy; usenp.int_or a fixed-width type.Is `np.int` removed in NumPy 1.24+, and what is the recommended replacement for annotations like `NDArray[np.int]`?Also applies to: 70-70
bindings/py/src/img2num_pybind.cpp (5)
9-9: LGTM!
12-25: LGTM!
43-61: LGTM!Also applies to: 73-89, 102-120, 134-152, 168-190, 210-232, 248-268, 271-308, 366-384
244-244: LGTM!
357-358: LGTM!bindings/py/Doxyfile.internal (1)
1-14: LGTM!bindings/py/doxygen/home_page.dox (1)
1-49: LGTM!Also applies to: 75-95
docs/docs/internal/bindings/py/index.md (1)
1-49: LGTM!Also applies to: 74-78
docs/docs/internal/bindings/py/api-reference.md (1)
1-23: LGTM!docs/scripts/doxygen.js (1)
14-14: LGTM!Also applies to: 32-36
bindings/js/doxygen/home_page.dox (1)
26-26: LGTM!docs/docs/internal/index.md (1)
32-35: LGTM!Also applies to: 37-37
README.md (1)
43-43: LGTM!
| return {.bilateral_filter{.sigma_spatial = c.bilateral_filter.sigma_spatial, | ||
| .sigma_range = c.bilateral_filter.sigma_range}, | ||
| .kmeans{.k = c.kmeans.k, .max_iter = c.kmeans.max_iter}, | ||
| .min_cluster_area = c.min_cluster_area, | ||
| .color_space = c.color_space}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify C++ standard/toolchain config and scan for this initializer pattern.
rg -n "CMAKE_CXX_STANDARD|set\\(CMAKE_CXX_STANDARD" CMakeLists.txt **/CMakeLists.txt
rg -nP '\.\w+\s*\{' bindings/c/src/cimg2num.cppRepository: Ryan-Millard/Img2Num
Length of output: 586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check for any C++ standard overrides beyond the first matches.
rg -n "CMAKE_CXX_STANDARD|CMAKE_CXX_FLAGS|std=c\\+\\+|gnu\\+\\+|CXX_STANDARD|target_compile_features|set\\(CXX_STANDARD" \
**/CMakeLists.txt **/*.cmake CMakeLists.txt || true
# 2) See whether this (non-`=`) nested designated-initializer pattern appears elsewhere.
rg -nP '\.\w+\s*\{[^}]*\.' bindings/c/src/cimg2num.cpp || true
rg -nP '\.\w+\s*\{' bindings/c/src/cimg2num.cpp || true
# 3) Confirm there are only these two specific return statements (context).
rg -n "return \\{\\." -n bindings/c/src/cimg2num.cpp && sed -n '1,140p' bindings/c/src/cimg2num.cpp || trueRepository: Ryan-Millard/Img2Num
Length of output: 5350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find other occurrences of ".name { ... }" style aggregates in C++.
# Use narrower regex to avoid too many brace-init false positives.
rg -nP '\.\w+\s*\{' . || true
# 2) Look for any -std= or CXXFLAGS/EM_FLAGS overrides that could bump the standard.
rg -n "(-std=c\\+\\+\\d+|CXXFLAGS|EMSDK|EMSCRIPTEN|EM_CXXFLAGS|EM_FLAGS|EM_LDFLAGS)" . || true
# 3) If the repo has JS/WASM build tooling, search for C++ standard settings there too.
rg -n "c\\+\\+\\d+|CXX_STANDARD|CMAKE_CXX_STANDARD|-std=" . || trueRepository: Ryan-Millard/Img2Num
Length of output: 24179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for C++20-style designated initializers with '=' inside brace-init-lists.
# Limit to C/C++ sources to avoid noise.
rg -nP '\{\s*\.\w+\s*=\s*' --glob '*.{c,cc,cpp,cxx,h,hpp,hxx}' . || trueRepository: Ryan-Millard/Img2Num
Length of output: 502
🌐 Web query:
C++ designated initializer syntax without '=' .member{...} is it valid and which compilers/standards support it?
💡 Result:
Yes, the designated initializer syntax using braces without an equals sign, such as.member{value}, is valid in C++20 and later standards [1][2]. In C++20, designated initializers were introduced for aggregate initialization [1][3]. The syntax allows for both the assignment-style (.member = value) and the brace-initialization-style (.member{value}) [2]. Key points regarding this syntax: - Standard Support: This is a standard feature of C++20 and subsequent versions [1][2]. - Aggregate Initialization: It is strictly limited to aggregate types [1][2]. - Syntax Flexibility: You can use either.member = value or.member{value} [2]. These can also be mixed within the same initializer list if desired [2]. - Compiler Support: Modern compilers that support the C++20 standard (such as GCC, Clang, and MSVC) support this syntax when the appropriate standard flag (e.g., -std=c++20) is enabled [2][3]. Note that this differs from the C programming language, which requires the equals sign (.member = value) and does not support the brace-only syntax for designated initializers [1][4]. Additionally, C++ designated initializers have stricter requirements than C, such as requiring members to be initialized in their declaration order [1][2].
Citations:
- 1: https://en.cppreference.com/cpp/language/aggregate_initialization
- 2: https://www.cppstories.com/2021/designated-init-cpp20/
- 3: https://www.youtube.com/watch?v=44rs_hX1dxE
- 4: https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "CMAKE_CXX_STANDARD\s*20|CMAKE_CXX_STANDARD\s*2[0-9]|CXX_STANDARD\s*20|target_compile_options\\(|CMAKE_CXX_FLAGS.*-std=|CXXFLAGS.*-std=|-std=c\\+\\+20|-std=gnu\\+\\+20|c\\+\\+20|EM.*CXXFLAGS|EM_FLAGS" . --glob '*.{cmake,CMakeLists.txt,sh,txt,md,js,ts,py,c,h,cc,cpp,cxx,hpp,hxx}'Repository: Ryan-Millard/Img2Num
Length of output: 1279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "struct\\s+ImageToSvgConfig|ImageToSvgConfig\\b" core/include bindings/c/include bindings/c/src bindings/c/CMakeLists.txt --glob '*.{h,hpp,cpp,c,cc}'Repository: Ryan-Millard/Img2Num
Length of output: 1202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the C++ struct definition for ImageToSvgConfig
nl -ba core/include/img2num.h | sed -n '1,140p'Repository: Ryan-Millard/Img2Num
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n core/include/img2num.h | sed -n '1,160p'Repository: Ryan-Millard/Img2Num
Length of output: 3868
Fix C++17 build break in bindings/c/src/cimg2num.cpp
return {.member{...}, ...} uses C++20 designated-initializer syntax (.field = ... / brace form), but this repo forces CMAKE_CXX_STANDARD 17, so it can fail to compile on C++17 toolchains. Replace with C++17-compatible aggregate initialization (or explicit assignments) for img2num::ImageToSvgConfig.
Proposed fix
// to_cpp(...)
return {{c.bilateral_filter.sigma_spatial, c.bilateral_filter.sigma_range},
{c.kmeans.k, c.kmeans.max_iter},
c.min_cluster_area,
c.color_space};
// to_c(...)
return {{cpp.bilateral_filter.sigma_spatial, cpp.bilateral_filter.sigma_range},
{cpp.kmeans.k, cpp.kmeans.max_iter},
cpp.min_cluster_area,
cpp.color_space};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/c/src/cimg2num.cpp` around lines 11 - 15, The return uses C++20
designated-initializer syntax which breaks C++17 builds; update the construction
of img2num::ImageToSvgConfig in the to_cpp/to_c conversion functions to use
C++17-compatible aggregate initialization or explicit member assignment instead
of `.member{...}` syntax — e.g. replace `return {.bilateral_filter{...},
.kmeans{...}, .min_cluster_area, .color_space}` with a plain braced aggregate
`return {{...}, {...}, ..., ...}` or build a local ImageToSvgConfig and assign
bilateral_filter, kmeans, min_cluster_area, color_space members before
returning.
| pybind11::array_t<int32_t, pybind11::array::c_style> out_labels( | ||
| {(ssize_t)height, (ssize_t)width}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use static_cast instead of C-style casts.
Lines 200-201 use C-style casts (ssize_t) while lines 203-206 correctly use static_cast<>. For consistency with C++ best practices and the Google style guide, use static_cast<ssize_t>() here as well.
♻️ Proposed fix
-pybind11::array_t<int32_t, pybind11::array::c_style> out_labels(
- {(ssize_t)height, (ssize_t)width});
+pybind11::array_t<int32_t, pybind11::array::c_style> out_labels({
+ static_cast<ssize_t>(height), static_cast<ssize_t>(width)});As per coding guidelines, follow Google style guide for C/C++ code formatting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bindings/py/src/img2num_pybind.cpp` around lines 200 - 201, Replace the
C-style casts on the pybind11::array_t construction for out_labels and use C++
style casts: change {(ssize_t)height, (ssize_t)width} to use
static_cast<ssize_t>(height) and static_cast<ssize_t>(width) so the
pybind11::array_t<int32_t, pybind11::array::c_style> out_labels initialization
follows the project’s Google C++ style and matches the static_cast usage in the
surrounding code.
| ## Code of Conduct | ||
|
|
||
| Please review and adhere to our [Code of Conduct](./code-of-conduct.md) to help foster an open and welcoming environment. | ||
| Please review and adhere to our [Code of Conduct](/docs/contributing/code-of-conduct.md) to help foster an open and welcoming environment. |
There was a problem hiding this comment.
Use a docs route instead of a .md URL for Code of Conduct.
This link likely bypasses Docusaurus routing and can 404 in site navigation. Point it to the docs route path instead.
Proposed fix
-Please review and adhere to our [Code of Conduct](/docs/contributing/code-of-conduct.md) to help foster an open and welcoming environment.
+Please review and adhere to our [Code of Conduct](/docs/next/contributing/code-of-conduct) to help foster an open and welcoming environment.As per coding guidelines: "docs/docs/**: All documentation must follow Docusaurus conventions ... Verify valid frontmatter ... and correct sidebar wiring."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Please review and adhere to our [Code of Conduct](/docs/contributing/code-of-conduct.md) to help foster an open and welcoming environment. | |
| Please review and adhere to our [Code of Conduct](/docs/next/contributing/code-of-conduct) to help foster an open and welcoming environment. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/contributing/index.md` at line 13, Replace the hardcoded .md file
URL in the "Code of Conduct" link in docs/docs/contributing/index.md (the link
text "Code of Conduct" currently pointing to
/docs/contributing/code-of-conduct.md) with the Docusaurus doc route (remove the
.md extension), e.g. /docs/contributing/code-of-conduct or
/docs/contributing/code-of-conduct/ so the site router resolves it correctly;
update the link target and confirm the referenced document has valid frontmatter
and is wired into the sidebar.
| | Binding | Status | Docs | | ||
| | :-------------------- | :------------ | :------------------------------------------------------------- | | ||
| | **JavaScript (WASM)** | ✅ Production | [JS API Reference](/docs/next/js/js-api-reference) | | ||
| | **C++** | ✅ Production | [C++ API Reference](/docs/next/cpp/cpp-api-reference) | | ||
| | **C** | ✅ Production | [C API Reference](/docs/next/c/c-api-reference) | | ||
| | **Python** | 🟡 Early | [Python API Reference](/docs/next/python/python-api-reference) | |
There was a problem hiding this comment.
Fix likely broken API binding doc routes in the table.
These paths appear inconsistent with the documented file structure and likely resolve to 404s (js-api-reference, cpp-api-reference, etc.). Please point them to the canonical docs routes (e.g., /docs/next/js/api-reference, /docs/next/cpp/api-reference, /docs/next/c/api-reference, /docs/next/python/api-reference).
As per coding guidelines, "docs/docs/**: All documentation must follow Docusaurus conventions (docs/docs/ folder structure)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/index.md` around lines 29 - 34, The table in docs/docs/index.md
contains broken links for the API references (the link text tokens like [JS API
Reference], [C++ API Reference], [C API Reference], [Python API Reference]) that
currently point to paths such as /docs/next/js/js-api-reference,
/docs/next/cpp/cpp-api-reference, etc.; update each anchor to the canonical
Docusaurus routes: change the JS link to /docs/next/js/api-reference, C++ to
/docs/next/cpp/api-reference, C to /docs/next/c/api-reference, and Python to
/docs/next/python/api-reference so the table points to the correct docs
locations.
| ```python title="Applying basic image processing operations using the Python API" | ||
| import numpy as np | ||
| from img2num import ( | ||
| gaussian_blur_fft, | ||
| invert_image, | ||
| threshold_image, | ||
| image_to_svg, | ||
| ) | ||
|
|
||
| # Load image as uint8 numpy array (H, W, C) | ||
| image = np.zeros((256, 256, 3), dtype=np.uint8) | ||
|
|
||
| # Apply Gaussian blur | ||
| blurred = gaussian_blur_fft(image, sigma=1.5) | ||
|
|
||
| # Invert colors | ||
| inverted = invert_image(blurred) | ||
|
|
||
| # Threshold | ||
| thresholded = threshold_image(inverted, num_thresholds=4) | ||
|
|
||
| # SVG conversion | ||
| svg_str = image_to_svg(thresholded) | ||
| ``` |
There was a problem hiding this comment.
Usage example doesn't match actual binding signatures.
The example shows a simplified API (from img2num import ... with functions that don't require width/height), but the actual _img2num bindings (in img2num_pybind.cpp) require explicit width and height parameters for all functions. For example:
- Example shows:
gaussian_blur_fft(image, sigma=1.5) - Binding requires:
_img2num.gaussian_blur_fft(image, width, height, sigma=2.0)
This discrepancy will confuse developers. Either:
- Update the example to use the correct
_img2numlow-level API (matching home_page.dox lines 58-74), or - If a high-level
img2numwrapper exists, clarify that this example is for the wrapper and link to its implementation.
📝 Suggested fix to match actual bindings
-import numpy as np
-from img2num import (
- gaussian_blur_fft,
- invert_image,
- threshold_image,
- image_to_svg,
-)
+import numpy as np
+import _img2num
# Load image as uint8 numpy array (H, W, C)
image = np.zeros((256, 256, 3), dtype=np.uint8)
+width, height = image.shape[1], image.shape[0]
# Apply Gaussian blur
-blurred = gaussian_blur_fft(image, sigma=1.5)
+blurred = _img2num.gaussian_blur_fft(image, width, height, sigma=1.5)
# Invert colors
-inverted = invert_image(blurred)
+inverted = _img2num.invert_image(blurred, width, height)
# Threshold
-thresholded = threshold_image(inverted, num_thresholds=4)
+thresholded = _img2num.threshold_image(inverted, width, height, num_thresholds=4)
# SVG conversion
-svg_str = image_to_svg(thresholded)
+config = _img2num.ImageToSvgConfig()
+svg_str = _img2num.image_to_svg(thresholded, width, height, config)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/internal/bindings/py/index.md` around lines 50 - 73, The docs
example shows a high-level API (from img2num import gaussian_blur_fft,
invert_image, threshold_image, image_to_svg) but the actual bindings in
img2num_pybind.cpp expose low-level functions that require explicit width and
height (e.g., _img2num.gaussian_blur_fft(image, width, height, sigma)), causing
a mismatch; update the example to call the real binding signatures (use
_img2num.* functions and pass width and height to gaussian_blur_fft,
invert_image, threshold_image, image_to_svg) or explicitly state this snippet is
for a higher-level wrapper and link to that wrapper implementation, mentioning
the binding file _img2num and img2num_pybind.cpp so maintainers can reconcile
the two.
| - [Documentation](./docs/) | ||
| - [API usage](./api/) | ||
| - [Documentation](/docs/next/js/docs/) | ||
| - [API usage](/docs/next/js/js-api-reference/) |
There was a problem hiding this comment.
Fix API link target to match the actual docs route.
/docs/next/js/js-api-reference/ likely does not map to the provided docs/docs/js/api-reference.md page.
Proposed fix
-- [API usage](/docs/next/js/js-api-reference/)
+- [API usage](/docs/next/js/api-reference/)As per coding guidelines: "docs/docs/**: All documentation must follow Docusaurus conventions ... Verify valid frontmatter ... and correct sidebar wiring."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - [API usage](/docs/next/js/js-api-reference/) | |
| - [API usage](/docs/next/js/api-reference/) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/docs/js/index.md` at line 127, Update the broken API link in
docs/docs/js/index.md so it points to the actual page route for the JavaScript
API reference (either the site root route /docs/js/api-reference/ or the
relative path ./api-reference.md depending on your Docusaurus routing),
replacing the incorrect target /docs/next/js/js-api-reference/; verify the
target matches the doc file docs/docs/js/api-reference.md and that the
frontmatter/sidebar config exposes the route so the link resolves correctly.
|
Just to let you know, my PC is acting up, so I might not be able to work on this repository very much for like the next month or so. I'll still tey, though. That doesn't mean that I don't appreciate the contributions you make. You consistently add value and make this place much more enjoyable. I wanted to publish the Python package, but I'm not sure how feasible that will be on my laptop. I'll see tomorrow. |
No worries. Take your time! |
|
@Krasner, I have added a new branch - Please let me know if you have any troubles with it. Also, feel free to merge this PR into that branch whenever you feel happy do so. :) |
|
Cool! What do you suggest the PR workflow is in general? |
|
I think we still need your PR #398 to be merged for all the ci checks to take effect. |
Yes. That is basically it. I need to update the docs for this flow, but that is precisely the plan. flowchart TD
A[Whoever's branch] --> B[PR to dev]
B --> C[Code review and CI]
C --> D[Merge into dev]
D --> E[Integration testing on dev]
E --> F{Ready for release}
F -- No --> A
F -- Yes --> G["Merge dev into main<br />(fast-forward-only: --ff-only)"]
G --> H[Main updated]
H --> I[Release Please triggered]
I --> J[Release PR created or updated]
J --> K[Changelog and version bump]
K --> L[Git tag and GitHub release]
L --> M[Artifacts published]
M --> N[Optional sync main back to dev]
N --> D
|
Yes. I wanted to set up the releases first so I could more easily debug that setup. What is your timezone, by the way? It seems like we almost always miss each other. Mine is GMT+2. |
GMT -4 (-5 if daylight saving) so we're 6 or 7 hours apart |
|
Hi @Krasner. What's the status with this? |
Haven't worked on it more. But it's mostly complete. Had some trouble documenting packages/py vs bindings/py. The main issue is that packages/py creates I know it's a major pain to review... |
|
@Ryan-Millard because the Dockerfile has been updated to have users but right now in the ci build you see: I think you need to add |
|
/docker-build c5de571 |
🐳 Docker image built successfully!Image
Run it locally:
|
Yep that is the fix! |
We could change their names. They don't really matter in Python.
That would fix it, but it would also make future changes in the environment slightly more complex. I did it because it's theoretically safer, but I don't know how much safety it actually gives us - it is just a development environment after all. Which do you think I should do?
One thing I did notice that raised a red flag in this PR is that the LLM didn't realise that we use Doxygen. Since Doxygen doesn't integrate with Docusaurus very well (it outputs HTML, whereas Docusaurus is markdown-based), we use We'll just need to rectify that in this PR. It's kind of a hack, but it was more convenient that setting up and maintaining something that does: flowchart TD
A["Doxygen-generated XML"]
B["XML-to-markdown conversion too (e.g., doxybook2)"]
C["Markdown"]
A --> B
B --> C
The problem is that Doxygen is designed to be a standalone site and we don't use it that way. |
In the Dockerfile fix I did option 1 - gave USER |
Actually I think it used iframe correctly. Here is the new python bindings docs when I run docusaurus locally |
Did it move it? When I briefly looked through the changes on this PR, it showed that the page that previously had it changed. |
|
Hmm some of the links as I click thru are broken... |
2 participants
What was changed & why
Generate updated documentation using codex.
Specifically
ollama launch codexwith qwen3.6https://docs.ollama.com/integrations/codex
Fixes: #372
Changes
Testing & Verification
Additional Resources