⚡ Bolt: 최솟값 검색 성능 최적화 (O(N log N) -> O(N)) - #200
Conversation
`surveyFA.R`의 최솟값 검색 부분에서 `sort(x)[1]` 대신 `which.min(x)`를 사용하도록 최적화하였습니다. 이를 통해 불필요한 전체 배열 정렬 오버헤드를 줄이고 검색 성능을 O(N log N)에서 O(N)으로 개선했습니다.
|
👋 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: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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. 📝 WalkthroughWalkthrough
ChangessurveyFA 변경
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
`surveyFA.R`의 최솟값 검색 부분에서 `sort(x)[1]` 대신 `which.min(x)`를 사용하도록 최적화하였습니다. 이를 통해 불필요한 전체 배열 정렬 오버헤드를 줄이고 검색 성능을 O(N log N)에서 O(N)으로 개선했습니다.
`surveyFA.R`의 최솟값 검색 부분에서 `sort(x)[1]` 대신 `which.min(x)`를 사용하도록 최적화하였습니다. 이를 통해 불필요한 전체 배열 정렬 오버헤드를 줄이고 검색 성능을 O(N log N)에서 O(N)으로 개선했습니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/testthat/test-surveyFA.R`:
- Around line 90-134: Update the test fixture and calls around surveyFA so the
recovery failure is deterministic: use input data that still causes try_fit() to
fail after the pre-fit removal of raw$item6, or stub mirt::mirt to force that
failure. Ensure the autofix = TRUE case exercises item removal, and set unstable
= TRUE in the second surveyFA call to actually cover the unstable branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e9172e9-20c4-4fc3-a9e6-73f0cd55a1a5
📒 Files selected for processing (5)
.jules/bolt.mdR/surveyFA.Rtest_dummy.Rtest_validation.Rtests/testthat/test-surveyFA.R
💤 Files with no reviewable changes (2)
- test_validation.R
- test_dummy.R
| # Intentionally messy data to force failure on standard methods | ||
| raw <- as.data.frame( | ||
| matrix( | ||
| c(rep(1, 20), rep(0, 20), rbinom(160, 1, 0.5)), | ||
| ncol = 5 | ||
| ) | ||
| ) | ||
| names(raw) <- paste0("item", 1:5) | ||
| raw$item6 <- 0 # Constant column | ||
|
|
||
| # Force failure without autofix | ||
| expect_error( | ||
| suppressWarnings( | ||
| aFIPC::surveyFA( | ||
| data = raw, | ||
| autofix = FALSE, | ||
| forceUIRT = TRUE, | ||
| forceNormalEM = FALSE, | ||
| forceMHRM = TRUE, # Cover forceMHRM branch | ||
| unstable = FALSE, | ||
| SE = TRUE, | ||
| itemtype = "2PL", | ||
| maxItemRemovals = 2 | ||
| ) | ||
| ), | ||
| "could not estimate a valid model after bounded recovery attempts" | ||
| ) | ||
|
|
||
| # Trigger unstable branch and force NormalEM=FALSE | ||
| expect_error( | ||
| suppressWarnings( | ||
| aFIPC::surveyFA( | ||
| data = raw, | ||
| autofix = TRUE, | ||
| forceUIRT = TRUE, | ||
| forceNormalEM = FALSE, | ||
| forceMHRM = FALSE, | ||
| unstable = FALSE, | ||
| SE = TRUE, | ||
| itemtype = "2PL", | ||
| maxItemRemovals = 2 | ||
| ) | ||
| ), | ||
| "could not estimate a valid model after bounded recovery attempts" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- tracked files ---'
git ls-files | rg '(^|/)(surveyFA\\.R|test-surveyFA\\.R)$'
printf '%s\n' '--- test fixture and expectations ---'
test_file=$(git ls-files | rg '(^|/)tests/testthat/test-surveyFA\\.R$' | head -n1)
survey_file=$(git ls-files | rg '(^|/)R/surveyFA\\.R$' | head -n1)
printf 'test_file=%s\nsurvey_file=%s\n' "$test_file" "$survey_file"
sed -n '70,180p' "$test_file"
printf '%s\n' '--- surveyFA implementation ---'
sed -n '1,240p' "$survey_file"Repository: ContextualWisdomLab/aFIPC
Length of output: 185
🏁 Script executed:
#!/bin/bash
set -e
test_file=$(git ls-files | rg '(^|/)tests/testthat/test-surveyFA\.R$' | head -n1)
survey_file=$(git ls-files | rg '(^|/)R/surveyFA\.R$' | head -n1)
printf '%s\n' '--- test section ---'
sed -n '70,180p' "$test_file"
printf '%s\n' '--- surveyFA implementation ---'
sed -n '1,240p' "$survey_file"Repository: ContextualWisdomLab/aFIPC
Length of output: 9828
🏁 Script executed:
#!/bin/bash
set -e
survey_file=$(git ls-files | rg '(^|/)R/surveyFA\.R$' | head -n1)
printf '%s\n' '--- recovery and branch logic ---'
sed -n '220,430p' "$survey_file"
printf '%s\n' '--- test setup and skip scope ---'
test_file=$(git ls-files | rg '(^|/)tests/testthat/test-surveyFA\.R$' | head -n1)
sed -n '1,155p' "$test_file"
printf '%s\n' '--- deterministic matrix-shape probe ---'
python3 - <<'PY'
import random
values = [1] * 20 + [0] * 20 + [random.randint(0, 1) for _ in range(160)]
nrow, ncol = 40, 5
columns = [values[i*nrow:(i+1)*nrow] for i in range(ncol)]
for i, col in enumerate(columns, 1):
print(f"item{i}: unique={sorted(set(col))}, counts={{0:{col.count(0)},1:{col.count(1)}}}")
PYRepository: ContextualWisdomLab/aFIPC
Length of output: 6638
🏁 Script executed:
#!/bin/bash
set -e
survey_file=$(git ls-files | rg '(^|/)R/surveyFA\.R$' | head -n1)
test_file=$(git ls-files | rg '(^|/)tests/testthat/test-surveyFA\.R$' | head -n1)
sed -n '220,430p' "$survey_file"
sed -n '1,155p' "$test_file"
python3 - <<'PY'
import random
values = [1] * 20 + [0] * 20 + [random.randint(0, 1) for _ in range(160)]
for i in range(5):
col = values[i * 40:(i + 1) * 40]
print(i + 1, sorted(set(col)), col.count(0), col.count(1))
PYRepository: ContextualWisdomLab/aFIPC
Length of output: 6399
복구 실패를 결정적으로 재현하는 fixture를 사용하세요. raw$item6은 적합 전에 제거됩니다. matrix(...)는 item1–item5에 유효한 응답을 남깁니다. 따라서 forceMHRM = TRUE와 autofix = TRUE는 try_fit() 실패를 보장하지 않으며, 현재 테스트는 autofix 항목 제거도 검증하지 못합니다. 실패를 보장하는 입력 또는 mirt::mirt stub을 사용하세요. unstable 분기를 검증하는 두 번째 호출은 unstable = TRUE로 설정하세요.
🤖 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 `@tests/testthat/test-surveyFA.R` around lines 90 - 134, Update the test
fixture and calls around surveyFA so the recovery failure is deterministic: use
input data that still causes try_fit() to fail after the pre-fit removal of
raw$item6, or stub mirt::mirt to force that failure. Ensure the autofix = TRUE
case exercises item removal, and set unstable = TRUE in the second surveyFA call
to actually cover the unstable branch.
`surveyFA.R`의 최솟값 검색 부분에서 `sort(x)[1]` 대신 `which.min(x)`를 사용하도록 최적화하였습니다. 이를 통해 불필요한 전체 배열 정렬 오버헤드를 줄이고 검색 성능을 O(N log N)에서 O(N)으로 개선했습니다.
1 participant
💡 What:
R/surveyFA.R파일 내에서 p-value의 최솟값에 해당하는 항목 이름을 찾기 위해 사용되던names(sort(p_values, decreasing = FALSE))[1L]코드를names(which.min(p_values))로 변경하였습니다.🎯 Why:
sort(x)[1]을 사용하면 단순히 가장 작은 값 하나를 찾기 위해 전체 배열을 정렬해야 하므로 O(N log N)의 시간 복잡도를 갖습니다. 반면which.min(x)는 배열을 한 번 순회하여 최솟값의 인덱스를 찾으므로 O(N)의 시간 복잡도를 가져, 반복적인 호출 시 불필요한 연산 오버헤드를 줄이고 성능을 향상시킬 수 있습니다.📊 Impact: 최솟값 검색 성능이 O(N log N)에서 O(N)으로 개선되어 처리 속도가 향상됩니다. 특히, 모델 적합 실패 시 항목 제거 루프(fallback model iteration) 내에서 오버헤드 누적을 방지합니다.
🔬 Measurement: 기존 코드의 기능 보존 여부 검증을 위해 단위 테스트(
Rscript -e "testthat::test_dir('tests/testthat')")를 수행하여 모두 통과함을 확인했습니다.PR created automatically by Jules for task 7326988174887277971 started by @seonghobae
Summary by CodeRabbit
성능 개선
오류 검증 강화
테스트