Repository navigation
refactor(r): preserve observed category count without na.omit - #336
Conversation
|
👋 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. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. Walkthrough
Merge Risk:
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | 제목은 na.omit 없이 관측된 범주 수를 보존하는 리팩터링을 명확하게 설명하며, PR의 주요 변경 사항과 일치합니다. |
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create PR with unit tests
- Commit unit tests in branch
bolt-opt-na-omit-13167333535658435100
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
ℹ️ Recent review info
-
⚙️ Run configuration
-
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: e6d83716-793e-4d77-ada7-428c3f249771
📒 Files selected for processing (2)
-
.jules/bolt.md
R/aFIPC.R
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
ℹ️ Recent review info
-
⚙️ Run configuration
-
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID:
e6d83716-793e-4d77-ada7-428c3f249771
📒 Files selected for processing (2)
-
.jules/bolt.mdR/aFIPC.R
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
-
Keep R/aFIPC.R category-count change; remove scanner-config addition.
seonghobae
left a comment
There was a problem hiding this comment.
Social Jules FIX_THEN_MERGE (#336): deleted .github/workflows/codeql.yml; kept R/aFIPC.R category-count change; restored .jules/bolt.md to master. COMMENT only.
Current exact authority — 2026-09-06
master@f87c2324f1686135e57d8730c1b0b9420874f3008ed423b1acdfa527a25503a17c3c590cfbe9d39aScope
autoFIPC()currently counts the distinct observed non-missing response categories of paired common items withlength(stats::na.omit(unique(x))). This branch expresses the same protected contract assum(!is.na(unique(x))).The original PR described this as a measured runtime optimization even though no representative benchmark, allocation profile, or buyer-path latency evidence was attached. Unit/coverage success can establish semantic compatibility; it cannot establish a meaningful speedup, GC/allocation reduction, or an O(N) constant-factor benefit in the actual calibration workload. Those claims have therefore been removed from the merge authority for this lane.
Regression boundary
tests/testthat/test-optimization-equivalence.Rnow directly compares the candidate expression with the protectedna.omit(unique(...))expression over:NAplusNaNinput;The hand-computed contract is distinct observed non-missing categories. The unused factor level is intentionally not counted because the protected #56 behavior is observation-based; the older
length(levels(as.factor(x)))expression is compared only on ordinary response vectors where its semantics coincide. This prevents the regression test from silently rewriting current product semantics back to declared factor levels.The generated
.jules/bolt.mdaddition was unrelated repository-wide doctrine. Normal descendants restored that file to the exact protected blob7d3c603f9991196ce123271a6c1e129156f24fd8; no force push or destructive rebase was used.Performance acceptance
Do not promote this refactor as a performance improvement unless one unchanged exact head has a reproducible benchmark/profile using representative
autoFIPC()category vectors and form/item cardinalities, with the same R version, platform, package state and warm-up policy. At minimum record repeated wall-time distribution and allocation evidence for the expression or containing buyer-relevant path. Synthetic microtiming alone must not be extrapolated to end-to-end calibration speed.Until then the admissible claim is only: behavior-preserving expression simplification with explicit edge-case equivalence coverage.
Merge gate
Keep Draft until exact-head repository/security/SAST/current-review gates are terminal-valid. Predecessor results do not transfer after the head moved. Do not self-approve, create a no-op retrigger, weaken coverage/security gates, reintroduce generated Bolt doctrine, force-push, or destructively rebase.