Bugfix/review 20260703#7
Merged
Merged
Conversation
The OPTA/SPTA kernel evaluated all four contour directions against the pre-cycle state and deleted the unsafe pixels in one batch, so both sides of a two-pixel-thick bar were removed in the same pass and the skeleton disconnected (a 2x9 bar collapsed to its four corner pixels). Replace the single batch with four per-direction sub-iterations per cycle (mark-then-delete after each direction), restoring the sequential visibility the paper's two raster scans guarantee; the per-direction safety conditions are unchanged. Adds an 8-connected component counter and a 2px-bar connectivity test across all seven methods. Review finding F011, figureextract ecosystem review 2026-07-03. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All seven C++ kernels loop over interior pixels only, so foreground in the outermost row/column was never a deletion candidate and shapes touching the matrix edge kept 2-3px-thick blobs there, violating the documented one-pixel-wide skeleton contract (the package's own thinImage() example returned a 7-pixel barbell). thin() now zero-pads the binary matrix by one pixel on all sides before calling the kernel and crops back afterwards, one shared fix for all seven methods. The pinned thinImage example output is updated and an edge-touching bar test pins per-method row widths exactly. Review finding F012, figureextract ecosystem review 2026-07-03. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dt_manhattan and dt_chessboard seeded foreground pixels with a finite sentinel (nrow+ncol+1 and max(nrow,ncol)+1), which leaked out unrelaxed when the image contained no background pixel, producing plausible- looking but meaningless distances (7 and 4 on a 3x3) while euclidean correctly returned Inf. Seed with infinity instead, matching dt_euclidean and the documented distance-to-nearest-background contract; Inf + 1.0 propagates correctly through std::min. Adds all-foreground tests pinning Inf for all three metrics. Review finding F015, figureextract ecosystem review 2026-07-03. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
NA in any input storage mode (logical, integer, numeric) silently reached the C++ kernels as NA_integer_ (INT_MIN), corrupting neighbour sums and producing arbitrary skeletons far from the NA cell, with no error from any branch. as_binary_matrix() now checks anyNA() up front and stops with a message naming thinr and how to recode, which also covers distance_transform() and medial_axis() through the shared helper. Documents the constraint on all three entry points and adds NA-rejection tests for every storage mode. Review finding F016, figureextract ecosystem review 2026-07-03. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…line test No test asserted that thinning preserves the foreground component count, so the OPTA 2px-bar disconnection (F011) passed the entire suite. Add an 8-connected component-count-preservation property test across all seven methods on 2px/3px horizontal and vertical bars, L/T/ plus shapes, two disjoint blobs, and the thick ring (green now because the F011 fix landed first; it goes red on any future parallel-deletion regression). Strengthen the T-shape/plus-shape/disjoint-blob tests with exact component counts, and tighten the loosened line-collapse bound for holt (now 1 row; only OPTA's published N2 corner behaviour keeps 3). Review finding F013, figureextract ecosystem review 2026-07-03. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 2026-07-03 review fixes (F011/F012/F015/F016) changed user-visible behaviour but shipped without documentation. Document them at the source: - thin(): new @section Edge handling (border padding, F012) and @section Connectivity (one-component-in/out, 2px strokes stay connected, F011); NA now documented as rejected, not "not accepted". Dropped the inline "matches EBImage::thinImage" aside from the method list (the drop-in claim lives on thinImage(); see note below). - distance_transform(): document fully-foreground -> Inf across all metrics (F015) and that the frame is not an implicit background border; NA rejected loudly (F016). - New vignette("correctness-properties"): states the edge-handling, connectivity, empty-background, and invalid-input guarantees, why they follow from (not depart from) the published methods, and pins each with runnable examples. These properties are general across all seven methods, so they are documented once rather than per method. Explicit that F011 RESTORES SPTA fidelity rather than extending it -- none of the fixes is a novel algorithm. - NEWS.md: development-version section for all five review findings. - _pkgdown.yml: register the new article. Full suite green (512 pass). Follow-up for the maintainer: EBImage 4.54.0 exposes no thinImage(), so the package-wide "drop-in replacement for EBImage::thinImage()" positioning (README, ARCHITECTURE, thin_image.R) needs a maintainer decision and is left untouched here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The package described itself throughout as a "drop-in replacement for EBImage::thinImage()", but EBImage exposes no thinImage() (verified against EBImage 4.54.0: not exported, not internal, no man page; its morphology is dilate/erode/distmap/watershed only). The claim was factually false and, in DESCRIPTION and cran-comments.md, would have misrepresented the package to CRAN. Reframe accurately without changing behaviour: EBImage provides binary morphology but no thinning operator, so thinr *fills that gap* rather than replacing an EBImage function. thinImage() is retained and redocumented as what it actually is -- a convenience alias for thin(method = "zhang_suen"). The LGPL-3 rationale is corrected from "so EBImage can retire its in-tree thinning code" (there is none) to license-compatibility so an EBImage pipeline can gain thinning from thinr. Touches roxygen (thin_image.R, thinr-package.R + regenerated man/), DESCRIPTION, README, ARCHITECTURE, CLAUDE, cran-comments, and the choosing-a-method vignette. Distmap references are unchanged (that function does exist and behaves as documented). Suite green (512). Open for the maintainer: whether the camelCase thinImage() name and the ADR-007 "EBImage can depend on thinr" framing still earn their place now that the drop-in premise is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
thinImage() no longer serves a necessary purpose; thin() (default method Zhang-Suen) covers the same use. Drop the function, its manual page, NAMESPACE export, pkgdown reference entry, direct tests, and all documentation references. No backward-compatibility shim is provided. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Split four compound-semicolon statements onto separate lines and correct one continuation-line indent in the n_components() helper chunk, so lintr::lint_package() is clean. Logic is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.