refactor(r): preserve observed category count without na.omit - #336
refactor(r): preserve observed category count without na.omit#336seonghobae wants to merge 11 commits into
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. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Changes고유 비-NA 값 개수 계산
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to The common-item validation now counts unique non-missing values with an equivalent lower-overhead expression. Existing NA and calibration-path coverage supports unchanged behavior, with no remaining merge-blocking risk. 🚥 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 |
seonghobae
changed the title
1 participant
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.