Accuracy with Structural similarity index

[Artur-man]

No description provided.

[Artur-man]

@claude review

[Artur-man]

@claude review

[claude]

Claude finished @Artur-man's task in 3m 45s —— View job

---

Review: Accuracy with Structural Similarity Index

• Gather context (diff vs origin/dev, read C++/R sources)
• Review C++ changes (src/metrics.cpp, metrics.h, registration sources)
• Review R changes (R/registration.R, R/auxiliary.R, RcppExports)
• Post review

The single-histogram-per-tile design is genuinely nice — you get n, exact mu, exact var, and the two histogram metrics from one pass, and cv::multiply(..., CV_32F) for the cross term is the right call. The comments explaining why are excellent. Most of my findings below are about things that got dropped or disabled while porting inst/legacy/temp.cpp into src/metrics.cpp.

I could not compile — no OpenCV/R toolchain on this runner — so the compile-warning notes are by inspection.

---

High

1. The flat-tile guard is disabled, which likely breaks the headline metric — src/metrics.cpp:439

// if (std::max(var1, var2) < varThresh) { ++R.n_drop_flat; continue; }

Your own comment 60 lines above (metrics.cpp:371-376) explains exactly why this matters: "Empty background tiles would then report perfect agreement while measuring nothing, and in a tissue image most tiles are background." With the guard off, R.ssim is a pixel-weighted mean dominated by background tiles that all score ≈1.0, so a badly-aligned pair and a well-aligned pair will report nearly the same SSIM. Intersection is affected the same way.

Two knock-on effects:

• varFactor (param) and varThresh (metrics.cpp:377) are now unused → -Wunused-* warnings on CRAN builds.
• The commented line can't simply be uncommented: TiledMetrics (metrics.cpp:286-297) no longer has an n_drop_flat member — it was dropped in the port from temp.cpp:110.

If disabling was deliberate (e.g. it dropped too many tiles on your test data), please say so in a comment and delete varFactor/varThresh, rather than leaving the rationale comment contradicting the code. Fix this →

2. rebinAndNormalise lost its input assertion — src/metrics.cpp:337-340

const int group = 256 / nBins;
cv::Mat out = cv::Mat::zeros(nBins, 1, CV_32F);
for (int i = 0; i < 256; ++i) q[i / group] += p[i];

inst/legacy/temp.cpp:142 had CV_Assert(nBins > 0 && nBins <= 256 && (256 % nBins == 0)); and it did not survive the port. The doc comment still says "nBins must divide 256", but nothing enforces it. With nBins = 100, group = 2 and q[i/group] writes up to index 127 into a 100-element buffer — a heap overflow. With nBins > 256, group == 0 → integer division by zero. nBins is currently hardcoded to the default 32 so this isn't live, but it's a landmine for the first person who exposes it. Please restore the assert. Fix this →

3. inst/legacy/temp.cpp will ship inside the installed package

inst/ is copied verbatim into the installed package, so these 376 lines of scratch code — including a live // [[Rcpp::export]] annotation at temp.cpp:340 — end up on every user's disk. It's also a near-duplicate of src/metrics.cpp with non-static symbols of the same names (histStats, rebinAndNormalise, tiledAlignmentMetrics), so anyone who moves it into src/ gets ODR violations. Either delete it, or move it to a dev/ path (already in .Rbuildignore). Fix this →

---

Medium

4. Intersection and Bhattacharyya silently changed meaning — src/metrics.cpp:498-505

They were global, whole-mask histogram comparisons; they're now pixel-weighted means of per-tile comparisons. Same names, different scale — a 50×50 tile has ~2500 pixels over 32 bins, so per-tile Bhattacharyya carries far more small-sample noise than the global value did, and numbers won't be comparable to anything reported by 0.3.0. NEWS.md isn't touched in this PR; the 0.3.0 entry still describes the old behaviour. Worth a 0.3.1 entry noting both the new SSIM metric and the redefinition. Fix this →

5. The full tiled pass runs twice and 2/3 of its output is discarded — src/metrics.cpp:486-498

getTiledAlignmentMetrics builds all three maps plus three scalars and returns only ssim_map; getAlignmentMetrics then recomputes the identical struct for the scalars. In alignImages that's three tiled passes per registration (coarse metrics, fine metrics, map). Having getAlignmentMetrics take an out-param cv::Mat1d& — the pattern alignImages already uses for accuracyMatte — would drop one full pass. Fix this →

6. No validation of image type or size — src/metrics.cpp:357-365

The header comment promises CV_8U, same size, but nothing checks it. L = 255.0, C1/C2, and the 256-bin histogram range all silently produce garbage on a CV_32F input, and mismatched sizes throw from deep inside im2(roi) with an opaque message. A CV_Assert(im1.size() == im2.size() && im1.depth() == CV_8U && (mask.empty() || mask.size() == im1.size())); at the top would be cheap insurance.

7. Dead code left behind by the port

• ssimVals (metrics.cpp:399-400, 473) is reserved and filled but never read — temp.cpp:280-282 used it for ssim_median, which was dropped.
• n_drop_coverage / n_total are tracked but never surfaced to R.
• MatteMIMap now has zero live callers — every one of its six call sites is commented out. If it's being retired, remove it from matte_mi.cpp/.h; if it's coming back, drop the commented lines and gate it behind a parameter instead.

8. Internal symbols should have internal linkage — src/metrics.cpp:284-489

kNaN, struct TiledMetrics, histStats, rebinAndNormalise and tiledAlignmentMetrics are none of them declared in metrics.h, but all have external linkage. Wrapping them in an anonymous namespace documents the intent and lets the compiler inline more aggressively.

9. Six commented-out MatteMIMap(...) lines in accuracy.cpp:48, manual_registration.cpp:76,185,221, automated_registration.cpp:673,726. Git remembers; please delete them.

10. Two drop reasons share one counter — metrics.cpp:423 and :434 both do ++R.n_drop_coverage, but the second is a degenerate-histogram case, not a coverage failure.

---

Low / polish

11. Clamping negative SSIM to 0 discards the most informative signal — R/registration.R:3255 and :3766

tmp[tmp < 0] <- 0

SSIM ∈ [-1, 1], and negative values (structural anti-correlation) are precisely the strongest evidence of a bad local alignment — collapsing them into "same as mediocre" hides that in the Local SSIM tab. A diverging scale over [-1, 1] would read better. (The NaN tiles are handled correctly, incidentally: NaN < 0 is NA, and R permits NA in a logical subscript assignment when the RHS has length 1, so they're skipped and render as the default grey.)

12. Stale naming/comments. src/accuracy.cpp:50 still says // Matte MI metric; C++ still uses accuracyMatte and matte_map everywhere while R renamed to ssim_map. //. Report typo at metrics.cpp:507.

13. Document the SSIM variant. This is per-tile global SSIM with population (÷n) variance/covariance, not the 11×11 Gaussian-windowed, sample-variance (÷n−1) SSIM of Wang et al. Both choices are self-consistent and defensible, but the numbers won't match skimage.metrics.structural_similarity or cv::quality::QualitySSIM, and users will compare. One line in the docs/NEWS avoids the bug report.

14. Unrelated churn in man/VoltRon-methods.Rd — the [[<-,VoltRon,character,missing,ANY-method alias changes come from a different roxygen2/R version, not from this feature. Harmless, but noise in the diff.

15. No tests. There's no coverage for the new metric anywhere under tests/. A small unit test would pin down the interesting invariants cheaply: SSIM of an image against itself ≈ 1, symmetry in the argument order, all-NaN map when the mask is empty, and correct map dimensions (ceil(rows/50) × ceil(cols/50)).

---

Summary

The approach is sound and the map dimensions stay compatible with what MatteMIMap returned (both chunk at 50 px), so the Shiny plotting path should keep working. The blocking item for me is #1 — with the flat-tile guard off, the reported SSIM is probably close to constant across good and bad alignments on real tissue images, which defeats the purpose of the metric. #2 and #3 are quick, mechanical fixes. Everything else is cleanup.

I reviewed only; no changes were pushed.
· branch dev_ssmi

[Artur-man]

@claude check again