Skip to content

fix/258-getcommonChunk code in C++#342

Open
Nishita-shah1 wants to merge 5 commits into
masterfrom
fix/258-getCommonChunk()-to-C++
Open

fix/258-getcommonChunk code in C++#342
Nishita-shah1 wants to merge 5 commits into
masterfrom
fix/258-getCommonChunk()-to-C++

Conversation

@Nishita-shah1

@Nishita-shah1 Nishita-shah1 commented Jun 29, 2026

Copy link
Copy Markdown

Summary

Fixes #258 by adding a small C++ fast path for common-column detection inside getCommonChunk().

New file: src/get_common_chunk.cpp (~154 lines)

Exported function: common_value_for_group_subset_cpp(value_lists)

For one (column, group), given values split by chunk subset (e.g. each showSelected level), it checks whether those values are identical across chunks. This mirrors R's common_value_for_group_subset() (matrix / NA / scalar logic from PR #242).

Architecture: R still groups columns and groups via detect_common_value_dt(). C++ only accelerates the inner compare. No change to TSV format, public API, geom-.r, or animint.js.

What this PR adds

Item Detail
src/get_common_chunk.cpp C++ inner compare (~154 lines)
src/RcppExports.cpp, R/RcppExports.R Rcpp registration
R/z_animintHelpers.R common_value_for_group_subset() calls C++ when compiled; R fallback via options(animint2.use.cpp)
tests/testthat/test-compiler-getCommonChunk.R 11 unit tests (38 expectations)
vignettes/get-common-chunk-cpp.Rmd File-wise explanation and test guide
DESCRIPTION, NAMESPACE, NEWS.md, .ci/atime/tests.R Rcpp wiring, changelog, benchmark

C++ design (review-friendly)

Layer Where
Grouping (per column, per group, per chunk) R: detect_common_value_dt()
Inner compare (matrix + NA + scalar) C++: common_value_for_group_subset_cpp()
Assembly + TSV write R: getCommonChunk(), split_recursive(), saveChunks()

Internal C++ helpers: is_na_at, eq_at, scalar_at, wrap_common (correct R list shape for common column).

Documentation

Vignette: vignettes/get-common-chunk-cpp.Rmd

After install: vignette("get-common-chunk-cpp", package = "animint2")


Motivation

After PRs #242 and #255, getCommonChunk() is correct but the inner compare (matrix + NA handling) runs in R for every (column, group, chunk subset). This PR moves that hot inner loop to C++ while keeping grouping in R for a small, reviewable diff.


Design highlights

  1. R groups, C++ compares - ~154 lines of C++ vs ~320 line full-scan version
  2. SEXP element compare - correct NA handling across numeric / integer / logical / character
  3. Rf_Scalar* - Windows / R 4.5+ compatible C API
  4. wrap_common() - returns list(common = list(vector), is.common = ...) matching R shape
  5. No file I/O in C++ - R still runs dcast, split_recursive, saveChunks

Quick test

Requires Rtools on Windows for first load_all().

library(devtools)
load_all()
library(testthat)
library(data.table)

built <- data.table(
  group = rep(1:2, each = 4),
  showSelected = rep(c(1, 1, 2, 2), 2),
  x = rep(c(10, 20), each = 4),
  y = rep(c(1, 2), each = 4),
  fill = c("a", "a", "b", "b", "c", "c", "d", "d")
)
setkeyv(built, c("group", "showSelected"))

# Detection table (C++ inner compare when compiled)
dt <- animint2:::detect_common_value_dt(built, c("x", "y", "fill"), "showSelected")
print(dt[, .(col.name, group, is.common)])
# x, y -> TRUE; fill -> FALSE

# Full common / varied split
result <- animint2:::getCommonChunk(built, "showSelected", list(group = "group"))
names(result$common)   # "x", "y", "group"
result$varied          # nested list with fill per chunk

# Unit tests (no browser / servr needed)
testthat::test_file("tests/testthat/test-compiler-getCommonChunk.R")
# Expected: FAIL 0 | PASS 38

# Verify C++ compiled
"common_value_for_group_subset_cpp" %in% ls(asNamespace("animint2"), all = TRUE)

@tdhock

tdhock commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

thanks, but this seems overly complex (300+ lines of C++)
is it possible to simplify? (I was expecting <100 lines of C++ code to review, maybe I was being too optimistic though?)

@Nishita-shah1

Nishita-shah1 commented Jun 29, 2026 via email

Copy link
Copy Markdown
Author

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.30769% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.10%. Comparing base (e447188) to head (690b3c5).

Files with missing lines Patch % Lines
src/get_common_chunk.cpp 80.85% 18 Missing ⚠️
R/z_animintHelpers.R 86.11% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   69.14%   73.10%   +3.96%     
==========================================
  Files         163      165       +2     
  Lines        6002     8951    +2949     
==========================================
+ Hits         4150     6544    +2394     
- Misses       1852     2407     +555     
Flag Coverage Δ
javascript 81.25% <ø> (?)
r 69.32% <82.30%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

move getCommonChunk() to C++

2 participants