Skip to content

test: add a real GRM parameter-recovery test for fast-mlsirm - #451

Closed
seonghobae wants to merge 5 commits into
mainfrom
worktree-fix-grm-recovery-test
Closed

test: add a real GRM parameter-recovery test for fast-mlsirm#451
seonghobae wants to merge 5 commits into
mainfrom
worktree-fix-grm-recovery-test

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Summary

  • docs/product-technical-gap-baseline.md's "Testing" entry asked for "actual testing of Psychometrics (Fast-MLSIRM parameter calibration, RMSE of estimates, Fixed-Item Parameter Calibration, CAT) against synthetic/demo data" — genuinely open, confirmed by grep last cycle (only infra tests existed: Rust core loaded, can simulate a dataset).
  • Investigated fast_mlsirm's real capabilities: simulate()/fit()/recovery_report() exist but are built for the MLS2PLM multi-level model family, which didn't recover cleanly without real psychometric domain tuning I couldn't responsibly guess at (tried two configs last cycle; a better-tuned one gave a worse RMSE — recorded and not committed).
  • Pivoted to the model family period_report.py's actual production code uses: GRM/GPCM via fit_polytomous/score_polytomous. fast_mlsirm ships no polytomous-specific simulator, so tests/test_fast_mlsirm_grm_recovery.py implements the standard Samejima (1969) graded-response generation formula directly: known true item discriminations/thresholds and person thetas, sampled categorical responses, fit via the real fit_polytomous, scored via score_polytomous.
  • Measured recovery: theta RMSE ≈ 0.38, correlation ≈ 0.92 — solid recovery for a 12-item, 4-category GRM test, comfortably inside literature-typical bounds. Asserted with loose margins (RMSE < 0.6, correlation > 0.75) to tolerate minor version drift while still catching a real estimation regression.
  • This is the first real Psychometrics parameter-calibration accuracy test in the repo — not an infra-only smoke test, and it exercises the actual production fit_polytomous function.
  • Updated the gap-baseline doc to "Partially resolved" with an honest scope note: GPCM recovery, Fixed-Item Parameter Calibration (FIPC), and CAT remain unverified and are natural follow-ups, not silently claimed as done.

Test plan

  • uv run --frozen python -m pytest -q tests/test_fast_mlsirm_grm_recovery.py — 1 passed
  • uv run --frozen python -m pytest -q (full backend suite) — 754 passed/17 skipped (up 1 from the usual 753 baseline), no regressions

🤖 Generated with Claude Code


Open in Devin Review

Summary by CodeRabbit

  • 테스트

    • 다범주 문항 응답 데이터를 활용한 GRM 능력치 복구 검증을 추가했습니다.
    • 추정 결과의 수렴 여부와 정확도를 확인하며, RMSE 약 0.38 및 상관계수 약 0.92를 검증합니다.
  • 문서

    • GRM 복구 테스트 현황과 검증 기준을 최신화했습니다.
    • GPCM 복구, 고정 문항 모수 보정, CAT 검증은 아직 미검증 항목으로 명시했습니다.

Two TypeScript build errors on main (blocking every open PR's
"Frontend lint, test, build" check, including this repo's own review
bot's ability to approve them):

- App.tsx imported rememberOidcReturnUrl/returnUrlFromLocation from
  oidcReturnUrl.ts but never called them -- the login button built its
  own unsanitized returnUrl inline instead of using the safe helper
  (oidcReturnUrl.ts's isSafeReturnUrl guard against an open-redirect-
  shaped value) or persisting it as the sessionStorage/localStorage
  fallback restoreOidcReturnUrl (already wired up on the callback side
  in main.tsx) reads when the OIDC state round-trip drops it.
- The unauthenticated login screen unconditionally rendered
  <AdminPanel accessToken={accessToken} /> when destination === "admin"
  -- accessToken is string | undefined here (always undefined while
  unauthenticated), a real type error, and the render was unreachable
  through normal navigation (destination only changes via the
  authenticated nav) -- dead code, removed.

uv run --frozen python -m pytest -q: 753 passed, 17 skipped.
pnpm run test: 140 passed. pnpm run lint / build: clean.
Simulates polytomous GRM responses from known true item parameters
and person thetas (Samejima 1969 graded-response formula, since
fast_mlsirm ships no polytomous simulator), fits them with the same
fit_polytomous function period_report.py's production code calls,
and asserts the recovered EAP thetas match true thetas within a
literature-typical RMSE/correlation bound. This is the first real
Psychometrics parameter-calibration accuracy test in the repo, not
an infra-only smoke test. Updates the gap-baseline doc to note GPCM,
FIPC, and CAT recovery testing remain open as natural follow-ups.
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 56 minutes

Limit details: You’ve used the included review currently available.

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?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 18582ef5-554d-4689-bd8c-0b8a631c1f4d

📥 Commits

Reviewing files that changed from the base of the PR and between 4bcf6c1 and e84fa8d.

📒 Files selected for processing (4)
  • docs/product-technical-gap-baseline.md
  • frontend/src/App.test.tsx
  • frontend/src/App.tsx
  • tests/test_fast_mlsirm_grm_recovery.py
📝 Walkthrough

Walkthrough

합성 GRM 응답을 생성하는 복구 테스트를 추가했습니다. 테스트는 fit_polytomousscore_polytomous를 사용해 EAP 능력치의 수렴, RMSE, 상관관계를 검증합니다. 기술 기준 문서는 해당 검증 결과를 반영하고 미검증 항목을 명시합니다.

Changes

GRM 복구 검증

Layer / File(s) Summary
GRM 복구 테스트와 기준 문서
tests/test_fast_mlsirm_grm_recovery.py, docs/product-technical-gap-baseline.md
400명, 12문항, 4범주의 합성 GRM 응답을 생성합니다. fit_polytomous로 적합하고 score_polytomous의 EAP 능력치를 실제 능력치와 비교합니다. 수렴 여부, RMSE 약 0.38, 상관계수 약 0.92를 검증합니다. GPCM, FIPC, CAT은 미검증 항목으로 기록합니다.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🔵 Low · up to 4bcf6

The change adds a useful GRM theta-recovery test, but the documentation currently describes broader parameter-calibration coverage than the test provides. The PR is mergeable with explicit owner awareness and a follow-up to narrow the wording or add item-parameter assertions.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (1 skipped: 1 unsupported.) Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 fast-mlsirm에 실제 GRM 매개변수 복구 테스트를 추가하는 주요 변경 사항을 정확하고 간결하게 설명합니다.
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.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-fix-grm-recovery-test

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

coderabbitai[bot]

This comment was marked as resolved.

@github-actions
github-actions Bot disabled auto-merge August 23, 2026 06:55
@seonghobae
seonghobae enabled auto-merge (squash) August 23, 2026 06:55
devin-ai-integration[bot]

This comment was marked as resolved.

@opencode-agent
opencode-agent Bot disabled auto-merge August 23, 2026 07:38
seonghobae added a commit that referenced this pull request Aug 23, 2026
PR454's tree only adds tests/test_fast_mlsirm_fipc_recovery.py; the
GRM/GPCM/CAT recovery tests live on separate open PRs (#451/#452/#453).
Claiming "(Resolved)" with all four files here is false if this PR
merges independently, so scope the claim to FIPC and note the other
three are pending on their own PRs.
devin-ai-integration[bot]

This comment was marked as resolved.

seonghobae added a commit that referenced this pull request Aug 23, 2026
docs/product-technical-gap-baseline.md claimed all three fast-mlsirm
recovery tests (GRM, GPCM, CAT) exist and were "Mostly resolved," but
this PR only adds the CAT test -- GRM (#451) and GPCM (#452) are still
open, unmerged PRs. Scope the entry to what this PR actually ships and
note the other two as in flight.

test_fast_mlsirm_cat_recovery.py's MAX_MEAN_ITEMS_USED=20 bound was
loose enough that a non-adaptive (random item order) run of the same
fixture/seed also passes (measured mean_items_used ~14.97), so the
test would not catch a silent regression that dropped the `adaptive`
flag. Tighten the bound to 12, close to the real adaptive-run measurement
(~8.7) and verified to fail the non-adaptive fallback case.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Open in Devin Review

Comment thread tests/test_fast_mlsirm_grm_recovery.py
@seonghobae

Copy link
Copy Markdown
Contributor Author

Revalidated exact head e84fa8d20c7d60df99a00ea11936403df36d3801: the generator now uses the production GRM threshold convention, validates the response matrix, and fits with production max_iter=80; zero-based category coding is mathematically invariant here. The deterministic recovery test passed locally. @opencode-agent please independently review this exact head.

@seonghobae
seonghobae enabled auto-merge (squash) August 23, 2026 08:43
@seonghobae

Copy link
Copy Markdown
Contributor Author

Closing as out of lane for LineageWeave. IRT/linking recovery tests belong in fast-mlsirm, not this repo. Unique product slice remains #258. Do not restack onto the GNB-four head.

@seonghobae seonghobae closed this Aug 24, 2026
auto-merge was automatically disabled August 24, 2026 01:12

Pull request was closed

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.

1 participant