Skip to content

Enforce 1-based public trap_index across trap analysis and removal APIs#388

Open
charlesmartin14 wants to merge 2 commits intomasterfrom
codex/find-reason-for-failure-z1y3yj
Open

Enforce 1-based public trap_index across trap analysis and removal APIs#388
charlesmartin14 wants to merge 2 commits intomasterfrom
codex/find-reason-for-failure-z1y3yj

Conversation

@charlesmartin14
Copy link
Copy Markdown
Member

Motivation

  • Normalize the public-facing trap_index to be 1-based across analysis/artifact outputs and the removal API to avoid off-by-one confusion between internal SVD mode indices and user-visible indices.
  • Validate input trap_index values early in remove_traps to prevent passing zero-based indices into the ablation workflow.

Description

  • Update analyze_traps to enumerate trap mode indices starting at 1, record artifact trap_index values as 1-based, and adjust plotting and artifact assembly to expect 1-based trap_index values.
  • Modify _top_trap_component_row to treat the incoming trap_index as already-public (1-based) and stop adding +1 there.
  • Harden remove_traps and _trap_indices_from_traps_df with validation that trap_index values are >= 1, improve handling of per-layer traps selection and propagate selected_trap_indices through cached and non-cached removal paths, and make error messages consistent.
  • Adjust apply_remove_traps to validate non-empty, 1-based trap_indices and update the raised error message when invalid indices are detected.
  • Update tests to reflect the 1-based public index change: change expected values in test_top_trap_component_row, add test_analyze_traps_public_trap_indices_are_1_based and test_remove_traps_rejects_zero_based_public_trap_index, and simplify a test helper in test_trap_compute_efficiency.py.

Testing

  • Ran the modified unit tests including tests/test_analyze_traps.py, tests/test_trap_ablation_workflow.py, tests/test_trap_component_summary.py, and tests/test_trap_compute_efficiency.py which exercise artifact generation, remove workflow, component extraction, and SVD-call behavior respectively, and they all passed.
  • Verified that the new validation raises ValueError when zero-based public trap_index values are provided to remove_traps using the added test case which succeeded.
  • Confirmed that analyze/remove workflows produce and consume 1-based trap_index values via the added integration-style test which succeeded.

Codex Task

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant