Skip to content

docs: list shap_parallel module + test file in structural docs (#260)#261

Merged
zachtheyek merged 3 commits into
masterfrom
claude/docs-shap-parallel-260
Jul 24, 2026
Merged

docs: list shap_parallel module + test file in structural docs (#260)#261
zachtheyek merged 3 commits into
masterfrom
claude/docs-shap-parallel-260

Conversation

@claude

@claude claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to merged PR #259 (parallelizing RF SHAP explainability). PR #259 updated the one narrative doc that discussed the SHAP figures (docs/TRAINING_PIPELINE.md), but three structural docs enumerate source modules and unit-test modules verbatim and were out of date. This PR reconciles them:

  • CONTRIBUTING.md — Project Structure tree and Module Responsibilities table both grow a shap_parallel.py entry after rf_metrics.py (its natural sibling: another TF-free RF-adjacent helper called by train.py).
  • docs/ARCHITECTURE.md — Module table grows a shap_parallel.py row after rf_metrics.py, matching the neighbouring linked-path + role-sentence + See ... cross-ref style.
  • docs/TESTING.mdtests/unit/ layout block grows a test_shap_parallel.py entry next to the other train-adjacent test modules (after test_train_datasets.py).

Existing formatting conventions are preserved (column alignment in the CONTRIBUTING.md tree/table, ARCHITECTURE.md row style, TESTING.md sorted layout).

Per the issue's contract:

  • CLAUDE.md is deliberately left untouched — it carries only lean always-on rules and does not enumerate src/aetherscan/*.py modules or currently mention SHAP anywhere; nothing in it is made factually wrong by feat(train): parallelize RF SHAP explainability across CPU cores #259.
  • No .py source file, no SHAP logic, and no docs/TRAINING_PIPELINE.md change in this PR — those are out of scope; the TRAINING_PIPELINE.md subsection was already added by feat(train): parallelize RF SHAP explainability across CPU cores #259.
  • .claude/skills/aetherscan-repo-context/SKILL.md is NOT touched here — that path is read-only to Claude Code in CI. The required edit is spelled out verbatim below for a human (CODEOWNER) to apply locally.

SKILL.md edits (apply manually — .claude/ is read-only in CI)

.claude/skills/aetherscan-repo-context/SKILL.md's Project Structure tree (~lines 106-142) restates the source-module inventory and must also list shap_parallel.py. Because the @claude follow-up PR cannot write under .claude/, apply this edit verbatim locally.

Edit 1 — insert one line into the source tree, immediately after the existing rf_metrics.py line:

Find:

├── rf_metrics.py       # Pure RF eval-metric helper (persisted to training_stats by train.py)
├── models/{vae,random_forest}.py

Replace with:

├── rf_metrics.py       # Pure RF eval-metric helper (persisted to training_stats by train.py)
├── shap_parallel.py    # RF SHAP process-pool wrapper (TF-free; called by train.py)
├── models/{vae,random_forest}.py

(No other lines in SKILL.md need to change — it doesn't currently mention SHAP anywhere else, and no other statement in it is contradicted by this PR.)

Step-by-step for the human (CODEOWNER) apply

Because .claude/ is read-only to Claude Code in CI, the SKILL.md edit above cannot be pushed from this PR — it needs to be applied and committed by a human locally on this same branch. @zachtheyek (per CODEOWNERS: * @zachtheyek), please:

  1. Check this PR out locally:
    gh pr checkout <this-PR-number>
  2. Open .claude/skills/aetherscan-repo-context/SKILL.md and apply Edit 1 exactly as spelled out above (single-line insertion immediately after the rf_metrics.py line in the Project Structure tree).
  3. Stage, commit (GPG-signed), and push:
    git add .claude/skills/aetherscan-repo-context/SKILL.md
    git commit -S -m \"docs(skill): list shap_parallel.py in project structure tree\"
    git push
  4. Confirm the CI + review pass here, then merge.

Test plan

  • Ran pre-commit run --files CONTRIBUTING.md docs/ARCHITECTURE.md docs/TESTING.md locally — all applicable hooks (large files / case conflicts / merge conflicts / EOF / trailing whitespace / private key / branch guard / gitleaks) passed; the ruff hooks correctly skipped (no .py files staged).
  • Verified referenced files exist: src/aetherscan/shap_parallel.py and tests/unit/test_shap_parallel.py.
  • Visually diffed all three files to confirm the new entries slot in after rf_metrics.py / test_train_datasets.py respectively and preserve the surrounding tree indentation / table column alignment.
  • Confirm the automated claude-code-review first pass lands cleanly (I will wait, then address individual notes on this same PR per the CLAUDE.md review contract).
  • CODEOWNER (@zachtheyek) applies the SKILL.md edit on this same branch per the steps above.

Closes #260

Follow-up to #259 — the PR itself updated docs/TRAINING_PIPELINE.md, but
three structural docs enumerate source modules and unit-test modules
verbatim and are now out of date:

- CONTRIBUTING.md: add shap_parallel.py to the Project Structure tree and
  to the Module Responsibilities table (both after rf_metrics.py).
- docs/ARCHITECTURE.md: add a shap_parallel.py row to the Module table,
  matching the neighbouring linked-path + role-sentence + See-cross-ref
  style, after rf_metrics.py.
- docs/TESTING.md: add test_shap_parallel.py to the tests/unit/ layout
  block, next to the other train-adjacent test modules.

.claude/skills/aetherscan-repo-context/SKILL.md also restates the source
tree and needs an equivalent one-line insertion, but .claude/ is
read-only to Claude Code in CI — that edit is spelled out verbatim in
the PR body for a human (CODEOWNER) to apply locally.

CLAUDE.md carries only lean always-on rules and does not enumerate
modules, so it is intentionally left untouched.

Closes #260
@claude
claude Bot requested a review from zachtheyek as a code owner July 24, 2026 04:44
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jul 24, 2026
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Claude finished @claude[bot]'s task in 5m 41s —— View job


Review

Docs-only follow-up to #259 that reconciles the three structural inventories the merged PR left drifting. Diff is 4 additions across 3 files, no .py touched. Overall the change is factually correct and low-risk — approving direction, with two small nits (one worth acting on, one optional). Two inline comments posted.

  • Fetch base branch and inspect the PR diff
  • Verify referenced files exist (src/aetherscan/shap_parallel.py, tests/unit/test_shap_parallel.py — both present)
  • Cross-check the three doc edits against existing formatting/ordering conventions
  • Confirm the "no other structural doc mentions SHAP inventory" claim (verified — every other docs/*.md SHAP reference is narrative, not enumerative; docs/TRAINING_PIPELINE.md already carries aetherscan.shap_parallel from feat(train): parallelize RF SHAP explainability across CPU cores #259; CLAUDE.md still doesn't mention SHAP anywhere and needs no touch, consistent with the PR body)
  • Post inline comments + final summary

Findings

Correctness / factual accuracy — good.

  • src/aetherscan/shap_parallel.py (7.5 KB) and tests/unit/test_shap_parallel.py (4.4 KB) both exist at the paths referenced by the new entries.
  • One-line descriptions match what's actually in shap_parallel.py (process-pool wrapper, TF-free, called by train.py::_compute_or_load_shap_values).
  • The See [TRAINING_PIPELINE.md](TRAINING_PIPELINE.md) cross-ref in docs/ARCHITECTURE.md resolves — that doc's line 416 references aetherscan.shap_parallel and its lines 406–447 are the SHAP explainability performance subsection added by feat(train): parallelize RF SHAP explainability across CPU cores #259.
  • Placement after rf_metrics.py in CONTRIBUTING.md tree/table and docs/ARCHITECTURE.md table is consistent (the two are natural siblings — both TF-free RF-adjacent helpers called by train.py), and the tree/table indentation and column style are preserved.
  • The PR body's scope contract (no .py, no SHAP logic, no TRAINING_PIPELINE.md change, CLAUDE.md deliberately untouched, SKILL.md deferred to a human because .claude/ is read-only in CI) is honored by the diff.

Nits (both non-blocking):

  1. docs/ARCHITECTURE.md:62 — trailing padding overshoots the table's right edge by ~165 chars. Every other row in the Module table pads its terminal | to roughly column 439–440; this new row is 605 chars long and sticks out ~165 chars past the pack. pre-commit's trailing-whitespace doesn't catch it because the whitespace is before the terminal |, not at EOL. Renders identically in markdown (GitHub doesn't care about source column alignment), but it's a very obvious visual anomaly if you scroll the raw source and it breaks the surrounding table's own convention. Inline comment on line 62 has two concrete fix options (trim padding only, or also tighten the Role prose to fit the existing column width).

  2. docs/TESTING.md:50test_shap_parallel.py sits next to test_train_datasets.py but its more natural sibling is test_rf_metrics.py three lines down (both are TF-free RF-adjacent tests, and this PR co-locates their production counterparts in the other two docs). Purely a placement consistency call — not wrong where it is, but worth considering. Inline comment on line 50.

CONTRIBUTING.md — no issue. The Module Responsibilities table there already doesn't strictly pad rows to a single terminal-| column (individual rows terminate at content + one space + |, and several existing rows — benchmark.py, dashboard_cli.py, rf_metrics.py — are naturally wider than the majority), so the new shap_parallel.py row's width is consistent with existing style. The tree section (line 187) also aligns cleanly with the neighbouring column format.

SKILL.md instructions — the step-by-step for the human CODEOWNER to apply the SKILL.md edit locally is clear, exact, and correctly notes the .claude/ read-only-in-CI constraint. No action possible from this PR.

Recommendation

Fix nit 1 (the docs/ARCHITECTURE.md trailing padding — that one really is out of step with the rest of that table), consider nit 2, and this is good to merge. The SKILL.md edit is a separate human-only step tracked in the PR body checklist.
· Branch: claude/docs-shap-parallel-260

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up on why this assistant workflow (claude.yml) fired on PR open — the @claude follow-up PR cannot write... phrase in the SKILL.md-edits section of the PR body contains an @ immediately followed by claude, and per CLAUDE.md backticks do not neutralize that trigger (only the "@ claude" form with a space and double quotes does). Mildly ironic given the PR itself is about keeping structural docs in sync with a merged change; I'll open a small follow-up to reword my own PR body to use the safe form so the assistant isn't re-summoned on every future rebase/edit.

Nothing else for this assistant run to do here:

  • The PR body already spells out the CODEOWNER (@ zachtheyek) action item verbatim (single-line insertion into .claude/skills/aetherscan-repo-context/SKILL.md's Project Structure tree, immediately after the rf_metrics.py line), and that path is read-only to Claude Code in CI so this workflow couldn't push it even if I tried.
  • The separate claude-code-review workflow will run the code-review first pass; per the CLAUDE.md review contract I shouldn't preempt it from this run.

Standing by — I'll respond to the claude-code-review notes on this same PR when they land.

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Ack — this run was re-triggered by the same @ claude phrase in the PR body flagged in my previous comment (the follow-up reword is still pending, so any further edit/rebase will keep re-firing this workflow until then). claude-code-review is still IN_PROGRESS; no new action from this run, still standing by for its notes per the CLAUDE.md review contract.

Comment thread docs/ARCHITECTURE.md Outdated
| [`inference_viz.py`](../src/aetherscan/inference_viz.py) | End-of-run inference visualization suite (ED distributions, galleries, latent projection, summary card). |
| [`models/vae.py`](../src/aetherscan/models/vae.py), [`models/random_forest.py`](../src/aetherscan/models/random_forest.py) | Model definitions. See [`MODELS.md`](MODELS.md). |
| [`rf_metrics.py`](../src/aetherscan/rf_metrics.py) | Pure (TF-free) helper `compute_rf_eval_metrics()` — sklearn.metrics-based scalar RF eval metrics that `train.py` persists to `training_stats` (`model_name='rf'`) for the dashboard's RF tab. |
| [`shap_parallel.py`](../src/aetherscan/shap_parallel.py) | TF-free process-pool wrapper (`parallel_shap`) that chunks the RF SHAP passes across worker processes; each worker rebuilds a stock `TreeExplainer` on its chunk. Called by `train.py::_compute_or_load_shap_values`. See [`TRAINING_PIPELINE.md`](TRAINING_PIPELINE.md). |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Trailing padding is ~170 chars past every other row in this table. All other rows in the Module table (lines 46–70) pad their trailing | to roughly column 439–440 (I verified with ^.{450,}$ → only this line matches; ^.{440,}$ → this line + one other). This new row is 605 chars long, so its terminal | sticks out ~165 chars to the right of the rest of the table's right edge — the source-file column alignment convention the rest of the table follows is broken here.

Renders identically in markdown (GitHub doesn't care), but it's an obvious visual anomaly when scrolling the raw source and inconsistent with the neighbouring rf_metrics.py / db/db.py rows.

Two ways to fix, pick one:

  1. Trim the trailing padding so the terminal | lands at the same column as its siblings (~col 439). The Role sentence itself (264 chars) is longer than the widest neighbour (main.py / seeding.py around 230 chars), so the row will still be a touch wider than average, but it won't sit ~170 chars past the pack. Minimal-touch fix.
  2. Also tighten the Role prose so it fits within the Role column width used by the rest of the table (~350 chars). E.g. drop the redundant "parallel_shap" (it's the module's only public entry point, the See TRAINING_PIPELINE.md link already scopes it) and collapse the two sentences: "TF-free process-pool wrapper that chunks RF SHAP passes across workers; each worker rebuilds a stock TreeExplainer. Called by train.py::_compute_or_load_shap_values. See TRAINING_PIPELINE.md." — that's ~215 chars and comfortably fits the existing column.

Either is fine; option 1 is the smallest change if you want to move on.

Comment thread docs/TESTING.md Outdated
│ ├── test_run_state.py # manifest round-trip, stage machine transitions
│ ├── test_train_utils.py # get_latest_tag ladder, curriculum schedules, archiving
│ ├── test_train_datasets.py # batched generators: coverage, stratification, alignment
│ ├── test_shap_parallel.py # positive-class shape normalization + MP-vs-single-process byte-identity (all three passes)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Placement is defensible but not the most natural home. The PR body describes the tests/unit/ ordering as "sorted" — it isn't alphabetical, it's pipeline-phase-grouped, and by that grouping test_shap_parallel.py is a closer sibling of test_rf_metrics.py (line 53) than of test_train_datasets.py: both are TF-free RF-adjacent helpers, and their production-code counterparts (rf_metrics.py, shap_parallel.py) are adjacent in both CONTRIBUTING.md and docs/ARCHITECTURE.md in this same PR.

Alternative placement worth considering: move this line to immediately after test_rf_metrics.py on line 53, so the RF/SHAP siblings sit together in TESTING.md the same way their source modules sit together in the other two docs. Purely a consistency call — the current position isn't wrong.

Completes PR #261's punted SKILL.md edit (the CI bot can't write under .claude/;
applied here as the local CODEOWNER). Inserts shap_parallel.py after rf_metrics.py
in the Project Structure tree, matching the sibling doc updates in this PR.

Refs #260
- docs/ARCHITECTURE.md: trim the shap_parallel row + pad its trailing pipe so it
  matches the Module table's column alignment (was ~165 chars past the right edge).
- docs/TESTING.md: move test_shap_parallel.py next to test_rf_metrics.py (both are
  TF-free RF-adjacent helpers, matching where the source modules sit in
  CONTRIBUTING.md / ARCHITECTURE.md).

Refs #260
@zachtheyek
zachtheyek merged commit 47ace9e into master Jul 24, 2026
8 checks passed
@zachtheyek
zachtheyek deleted the claude/docs-shap-parallel-260 branch July 24, 2026 06:29
@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Internal change — no release note.

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

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DOCUMENTATION]: List new shap_parallel module + test file in structural docs (PR #259)

1 participant