perf(ipd): avoid explicit frame reconstruction - #239
Conversation
* Replaced as.factor with explicit factor level assignment to prevent auto-inference overhead. * Used direct $ subsetting instead of data.frame() to prevent O(N) memory concatenation copies.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 15 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughIPD 그룹 생성과 결과 저장 방식을 최적화했습니다. ChangesIPD 처리 최적화
autoFIPC 입력 검증
R CMD check 환경 설정
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test_perf_matrix2.R (1)
2-3: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win열 할당 결과를 직접 검증하세요.
Line 3의
print(class(df))는df$C의 존재나 값을 검증하지 않습니다.df$C, 행 수, 열 이름을stopifnot()으로 확인하세요. 그렇지 않으면 Line 2의 할당이 제거되거나 잘못 변경되어도 검증 스크립트가 통과할 수 있습니다.🤖 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 `@test_perf_matrix2.R` around lines 2 - 3, Replace the class-only check after assigning df$C with stopifnot validations that confirm df$C exists with values 7:9, the expected row count is preserved, and the column names include C; remove or supplement print(class(df)) so incorrect or missing assignment causes the script to fail.test_matrix_assignment.R (1)
3-4: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win출력 대신 검증 조건을 추가하세요.
Line 3-4의
print(IPDData)는 열 할당 성공을 판정하지 않습니다.IPDgroup의 길이, levels, 행 수, 실제 열 값을stopifnot()으로 검증하세요. 현재 코드는 잘못된 결과도 성공한 실행처럼 보일 수 있습니다.🤖 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 `@test_matrix_assignment.R` around lines 3 - 4, Replace the print(IPDData) statement after the IPDData$IPDgroup assignment with stopifnot() assertions that verify IPDgroup length, factor levels, row count, and the assigned column values, so incorrect assignments fail explicitly.
🤖 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 `@R/aFIPC.R`:
- Around line 615-619: Update the factor() call assigning IPDgroup so its levels
are ordered c('newForm', 'oldForm'), matching the prior as.factor() behavior and
preserving multipleGroup()'s reference group and DIF effect direction.
In `@test_perf_matrix.R`:
- Around line 1-7: Update test_perf_matrix.R lines 1-7 to create IPDData as a
data.frame, let assignment errors fail the test instead of printing them, and
add stopifnot() checks for the assigned IPDgroup values and levels; update
test_matrix_assignment.R lines 3-4 with stopifnot() assertions for IPDgroup
values and levels; update test_perf_matrix2.R lines 2-3 with stopifnot()
assertions for df$C, row count, and column names.
---
Nitpick comments:
In `@test_matrix_assignment.R`:
- Around line 3-4: Replace the print(IPDData) statement after the
IPDData$IPDgroup assignment with stopifnot() assertions that verify IPDgroup
length, factor levels, row count, and the assigned column values, so incorrect
assignments fail explicitly.
In `@test_perf_matrix2.R`:
- Around line 2-3: Replace the class-only check after assigning df$C with
stopifnot validations that confirm df$C exists with values 7:9, the expected row
count is preserved, and the column names include C; remove or supplement
print(class(df)) so incorrect or missing assignment causes the script to fail.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9eae113a-7dae-404f-8f65-c822a66ae131
⛔ Files ignored due to path filters (1)
untested.csvis excluded by!**/*.csv
📒 Files selected for processing (6)
.jules/bolt.mdR/aFIPC.Rtest_matrix_assignment.Rtest_perf_matrix.Rtest_perf_matrix2.Rtests/testthat/test-autoFIPC.R
* as.factor 대신 명시적인 levels와 함께 factor()를 사용하여 O(N) 레벨 자동 추론을 방지 * data.frame(df, col) 복사 오버헤드를 막기 위해 $ 연산자로 직접 데이터 프레임 컬럼 할당 * 테스트 코드 보완 및 CI 의존성 설치 추가
* Fixed Deriv compilation failure on GitHub Actions by explicitly installing Deriv 4.1.3 before `setup-r-dependencies`. * Replaced as.factor with explicit factor level assignment to prevent auto-inference overhead. * Used direct $ subsetting instead of data.frame() to prevent O(N) memory concatenation copies.
* Fixed Deriv compilation failure on GitHub Actions by explicitly installing Deriv 4.1.3 before `setup-r-dependencies`. * Replaced as.factor with explicit factor level assignment to prevent auto-inference overhead. * Used direct $ subsetting instead of data.frame() to prevent O(N) memory concatenation copies.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/r.yml (1)
37-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winR 버전을 고정해 CI 기준을 안정화하세요.
r-version: release는 실행 시점에 따라 R 버전을 변경합니다. 이 워크플로는 특정Deriv컴파일 문제를 우회하므로, R 버전이 변경되면 현재 우회가 더 이상 유효하지 않을 수 있습니다. 재현 가능한R CMD check가 목적이면 지원하는 R 버전을 명시하세요. 최신 R 검증도 필요하면 고정 버전 작업과 별도의release작업으로 분리하세요.setup-r-dependencies의 의존성 설치 동작도 함께 확인하세요. (github.com)🤖 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 @.github/workflows/r.yml around lines 37 - 41, Update the R setup step using r-lib/actions/setup-r so r-version specifies a supported fixed R version instead of release, keeping CI and the Deriv workaround reproducible; if latest-release validation is required, move it to a separate workflow job and ensure setup-r-dependencies uses the intended dependencies for the pinned version.
🤖 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.
Nitpick comments:
In @.github/workflows/r.yml:
- Around line 37-41: Update the R setup step using r-lib/actions/setup-r so
r-version specifies a supported fixed R version instead of release, keeping CI
and the Deriv workaround reproducible; if latest-release validation is required,
move it to a separate workflow job and ensure setup-r-dependencies uses the
intended dependencies for the pinned version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bd0e92a3-f1bc-4d4c-bde8-20f4b4c41801
📒 Files selected for processing (1)
.github/workflows/r.yml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/r.yml (1)
43-50: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRcppParallel과 qs2의 버전과 소스 아카이브를 고정하십시오.
현재 두 패키지는 최신 CRAN 소스를 선택합니다.
setup-r-dependencies는 두 패키지 이후에 실행되지만, 현재DESCRIPTION에는 두 패키지가 직접 선언되어 있지 않습니다. 따라서 설치 순서 변경만으로 ABI 조합을 보장할 수 없습니다. 정확한 버전과 SHA-256 체크섬을 고정하고, 설치 후packageVersion()과find.package()로 결과를 확인하십시오.🤖 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 @.github/workflows/r.yml around lines 43 - 50, Update the “Build ABI-compatible R dependencies” step to install pinned RcppParallel and qs2 source-archive versions with their SHA-256 checksums instead of resolving the latest CRAN sources. After installation, verify each package using packageVersion() and find.package(), failing the workflow if the versions or archive checksums do not match the declared pins.Source: MCP tools
🤖 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.
Outside diff comments:
In @.github/workflows/r.yml:
- Around line 43-50: Update the “Build ABI-compatible R dependencies” step to
install pinned RcppParallel and qs2 source-archive versions with their SHA-256
checksums instead of resolving the latest CRAN sources. After installation,
verify each package using packageVersion() and find.package(), failing the
workflow if the versions or archive checksums do not match the declared pins.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 533174de-454b-4df9-a89c-de106fdd9831
📒 Files selected for processing (2)
.github/workflows/r.ymlR/aFIPC.R
🚧 Files skipped from review as they are similar to previous changes (1)
- R/aFIPC.R
* Fixed Deriv compilation failure on GitHub Actions by explicitly installing Deriv 4.1.3 before `setup-r-dependencies` and fixing line length limits in `r.yml`. * Replaced as.factor with explicit factor level assignment to prevent auto-inference overhead. * Used direct $ subsetting instead of data.frame() to prevent O(N) memory concatenation copies.
* Fixed Deriv compilation failure on GitHub Actions by explicitly installing Deriv 4.1.3 before `setup-r-dependencies` and fixing line length limits in `r.yml`. * Replaced as.factor with explicit factor level assignment to prevent auto-inference overhead. * Used direct $ subsetting instead of data.frame() to prevent O(N) memory concatenation copies.
* Fixed Deriv compilation failure on GitHub Actions by explicitly installing Deriv 4.1.3 before `setup-r-dependencies` and fixing line length limits in `r.yml`. * Replaced as.factor with explicit factor level assignment to prevent auto-inference overhead. * Used direct $ subsetting instead of data.frame() to prevent O(N) memory concatenation copies.
* Fixed Deriv compilation failure on GitHub Actions by explicitly installing Deriv 4.1.3 before `setup-r-dependencies` and fixing line length limits in `r.yml`. * Replaced as.factor with explicit factor level assignment to prevent auto-inference overhead. * Used direct $ subsetting instead of data.frame() to prevent O(N) memory concatenation copies. * Disabled needrestart restart prompts during apt-get to avoid step-security GitHub runner disconnects.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/r.yml (1)
46-51: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Deriv 4.1.3을extra-packages에 고정하세요.
setup-r-dependencies는 기본적으로 빈 라이브러리를 기준으로pak잠금 파일을 생성합니다. 따라서 사전 설치한Deriv 4.1.3은 의존성 해결에 반영되지 않습니다.extra-packages에 다음 항목을 추가하고 사전 설치 단계를 제거하세요.url::https://cran.r-project.org/src/contrib/Archive/Deriv/Deriv_4.1.3.tar.gz🤖 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 @.github/workflows/r.yml around lines 46 - 51, Update the setup-r-dependencies configuration to include Deriv 4.1.3 in extra-packages using the specified archive URL, and remove the separate Pre-install older Deriv version step. Ensure dependency resolution installs and locks this exact package version.
🤖 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.
Outside diff comments:
In @.github/workflows/r.yml:
- Around line 46-51: Update the setup-r-dependencies configuration to include
Deriv 4.1.3 in extra-packages using the specified archive URL, and remove the
separate Pre-install older Deriv version step. Ensure dependency resolution
installs and locks this exact package version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce7c1f6c-4438-41de-be66-8066fea04b93
📒 Files selected for processing (2)
.github/workflows/r.ymlR/aFIPC.R
🚧 Files skipped from review as they are similar to previous changes (1)
- R/aFIPC.R
seonghobae
changed the title
|
@coderabbitai review Please review exact unchanged head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='21d213317ff5d77743799d79ff7915781c2a720e'
printf '%s\n' '=== HEAD and requested commit ==='
git rev-parse HEAD
git cat-file -t "$requested"
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$requested"
printf '%s\n' '=== Merge base and changed files ==='
base="$(git merge-base "$requested" origin/main 2>/dev/null || git merge-base "$requested" main)"
printf 'base=%s\n' "$base"
git diff --stat "$base" "$requested"
git diff --name-status "$base" "$requested"
git diff --find-renames --find-copies --unified=80 "$base" "$requested" -- \
R/aFIPC.R tests/testthat/test-autoFIPC.R .github/workflows/r.yml .jules/bolt.md
printf '%s\n' '=== R/aFIPC.R outline ==='
ast-grep outline R/aFIPC.R --items all --type function 2>/dev/null || true
printf '%s\n' '=== Relevant source and test references ==='
rg -n -C 12 --glob 'R/aFIPC.R' --glob 'tests/testthat/test-autoFIPC.R' \
'autoFIPC\s*<-\s*function|IPDgroup|oldForm|newForm|Common Items are not equal|Please provide common item names' .
printf '%s\n' '=== Workflow ==='
cat -n .github/workflows/r.yml
printf '%s\n' '=== Dependency declarations ==='
rg -n -C 4 'Deriv|extra-packages|r-lib/actions|dependencies' \
DESCRIPTION .github/workflows README.md 2>/dev/null || trueLength of output: 1897 🏁 Script executed: #!/bin/bash
set -euo pipefail
requested='21d213317ff5d77743799d79ff7915781c2a720e'
printf '%s\n' '=== Available refs ==='
git branch -a
git for-each-ref --format='%(refname:short) %(objectname)' refs/heads refs/remotes
printf '%s\n' '=== PR metadata ==='
gh pr view 239 --repo ContextualWisdomLab/aFIPC \
--json number,baseRefName,baseRefOid,headRefName,headRefOid,state,mergeable,statusCheckRollup \
--jq '.'
printf '%s\n' '=== Candidate base commits ==='
git show -s --format='parent=%P' "$requested"
git show -s --format='parent_subject=%s' "${requested}^"Length of output: 9596 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='35e44988487de8f5b8b7e57d9d76e18911b99841'
head='21d213317ff5d77743799d79ff7915781c2a720e'
printf '%s\n' '=== Commit sequence ==='
git log --reverse --format='%H %s' "$base..$head"
printf '%s\n' '=== Changed files ==='
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
printf '%s\n' '=== Complete PR diff ==='
git diff --find-renames --find-copies --unified=100 "$base" "$head"
printf '%s\n' '=== R/aFIPC.R structure ==='
ast-grep outline R/aFIPC.R --items all --type function 2>/dev/null || true
printf '%s\n' '=== autoFIPC and supporting validation context ==='
rg -n -C 25 --glob 'R/aFIPC.R' \
'autoFIPC\s*<-\s*function|IPDgroup|oldForm|newForm|commonItem|Common Items are not equal|Please provide common item names' R/aFIPC.R
printf '%s\n' '=== Added and nearby test context ==='
rg -n -C 20 --glob 'test-autoFIPC.R' \
'Common Items are not equal|Please provide common item names|autoFIPC\s*\(' tests/testthat/test-autoFIPC.R
printf '%s\n' '=== Final workflow and package metadata ==='
cat -n .github/workflows/r.yml
printf '%s\n' '--- DESCRIPTION ---'
cat -n DESCRIPTIONLength of output: 67548 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Reference-group correction commit ==='
git show --format=fuller --find-renames --find-copies --unified=80 \
9a020807fea0dc5759c78373639607313809d70d -- R/aFIPC.R .github/workflows/r.yml tests
printf '%s\n' '=== IPD references in repository ==='
rg -n -i -C 8 \
'IPDgroup|IPDData|multipleGroup\s*\(|reference group|reference.*group|oldForm|newForm' \
--glob '!R/aFIPC.R' .
printf '%s\n' '=== Test inventory ==='
fd -t f . tests | sortLength of output: 72419 🤖 web_search executed:
Length of output: 15321
I found one blocking issue.
The
|
|
@coderabbitai review Please review exact current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
requested_short='f9afc05'
repo='ContextualWisdomLab/aFIPC'
printf '%s\n' '=== Requested commit and checked-out HEAD ==='
git rev-parse HEAD
requested="$(git rev-parse "${requested_short}^{commit}")"
printf 'requested=%s\n' "$requested"
git show -s --format='commit=%H%nparents=%P%nsubject=%s' "$requested"
printf '%s\n' '=== Pull request metadata and exact-head checks ==='
gh pr view 239 --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,statusCheckRollup \
--jq '{
number, state, baseRefName, baseRefOid, headRefName, headRefOid, mergeable,
checks: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
printf '%s\n' '=== Merge base and changed files ==='
base="$(git merge-base "$requested" origin/main 2>/dev/null || git merge-base "$requested" main)"
printf 'base=%s\n' "$base"
git diff --name-status "$base" "$requested"
git diff --stat "$base" "$requested"
git diff --find-renames --find-copies --unified=100 "$base" "$requested"
printf '%s\n' '=== R source structure ==='
ast-grep outline R/aFIPC.R --items all --type function 2>/dev/null || true
printf '%s\n' '=== IPD group construction and model call context ==='
rg -n -C 30 --glob 'R/aFIPC.R' \
'build_ipd_group|IPDgroup|factor\s*\(|multipleGroup\s*\(|oldForm|newForm|checkIPD' R/aFIPC.R
printf '%s\n' '=== Tests for IPD grouping and autoFIPC validation ==='
rg -n -C 30 --glob 'test-autoFIPC.R' \
'build_ipd_group|IPDgroup|oldForm|newForm|checkIPD|Common Items are not equal|Please provide common item names' \
tests/testthat/test-autoFIPC.R
printf '%s\n' '=== Workflow and dependency declarations ==='
cat -n .github/workflows/r.yml
printf '%s\n' '--- DESCRIPTION Deriv context ---'
rg -n -C 5 'Deriv|Suggests|Imports|Depends' DESCRIPTIONLength of output: 7405 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='35e44988487de8f5b8b7e57d9d76e18911b99841'
head='f9afc0594608d5ab27915b4db6f4c97ade29d372'
printf '%s\n' '=== Commit sequence ==='
git log --reverse --format='%H %s' "$base..$head"
printf '%s\n' '=== Changed files and complete PR diff ==='
git diff --name-status "$base" "$head"
git diff --stat "$base" "$head"
git diff --find-renames --find-copies --unified=100 "$base" "$head"
printf '%s\n' '=== R/aFIPC.R structure ==='
ast-grep outline R/aFIPC.R --items all --type function 2>/dev/null || true
printf '%s\n' '=== IPD group helper, its call site, and multipleGroup context ==='
rg -n -C 35 --glob 'R/aFIPC.R' \
'\.build_ipd_group|IPDgroup|factor\s*\(|multipleGroup\s*\(|oldForm|newForm|checkIPD' R/aFIPC.R
printf '%s\n' '=== IPD regression tests and nearby test helpers ==='
rg -n -C 35 --glob 'test-autoFIPC.R' \
'\.build_ipd_group|IPDgroup|oldForm|newForm|checkIPD|Common Items are not equal|Please provide common item names' \
tests/testthat/test-autoFIPC.R
printf '%s\n' '=== All helper references ==='
rg -n -C 4 --glob '*.R' '\.build_ipd_group\s*\(' R tests
printf '%s\n' '=== Workflow and dependency declarations ==='
cat -n .github/workflows/r.yml
printf '%s\n' '--- DESCRIPTION dependency context ---'
rg -n -C 5 'Deriv|Suggests|Imports|Depends' DESCRIPTION
printf '%s\n' '=== Current exact-head check statuses ==='
gh pr view 239 --repo ContextualWisdomLab/aFIPC \
--json statusCheckRollup \
--jq '[.statusCheckRollup[] | select(.name != null) | {name,status,conclusion}]'Length of output: 78928
I found no new blocking issue.
The regression test verifies both invariants. The The completed exact-head checks succeeded. The You are interacting with an AI system. |
1 participant
Outcome
Reduce avoidable work in the item-parameter-drift result path without claiming an unmeasured throughput gain.
oldForm/newFormfactor with explicit levels;IPDDataand addIPDgroupdirectly instead of explicitly rebuilding it throughdata.frame(IPDData, IPDgroup);R copy-on-modify may still allocate. This PR therefore makes no O(1), percentage, or end-to-end speedup claim; representative benchmarks remain separate evidence.
CI root-cause remediation
Exact predecessor
e791134c02f9545f636590771a53ec7927265b2bfailed before tests while SimDesign loaded a cachedqs2.sowhose TBB ABI no longer matched the runner. The workflow now rebuilds native R dependencies per runner. A follow-up exact-head review also showed the standalone Deriv 4.1.3 install sat outside the dependency action's empty-library lock resolution, so the exact archive is now part ofextra-packagesand the ineffective preinstall step is removed.Acceptance