Repository navigation
docs(dp): verify noise_multiplier is independently destructive (BL-23 follow-on) - #293
Conversation
|
Thanks, Abidoye. This overlaps with #247/#263, which already covers the clipping-default verification. Abraham also removed Track C from #279 for that reason. #263 now addresses the requested reference-data checks and sets the default to 10; this PR independently changes it to 15. Could we reconcile the two before merging? I suggest keeping #263 as the existing scoped change. If the additional experiment supports a further adjustment, please include a reproducible script and exact settings so we can assess it as a separate follow-up. Otherwise, we should narrow the empirical claims. This isn’t a finding that 15 is inherently incorrect. |
|
Reviewed against Claimed: Verified — manifest change: Verified — parameter count and The document's "~109K parameters, √D ≈ 330, aggregate noise swamps signal" claim holds exactly. The Verified — trained weight norm cross-reference: Verified — clipping-norm sweep inflection: the sweep table shows accuracy flat from Verified — Finding 2 is documented but not acted on: Verified — BL-23 update: status field changes from Verified — audit document placement: Verified — issue No. 279 is open: title "DP: fix Laplace calibration (F4) and label post-training mechanisms experimental (F5)", state OPEN. The PR closes Track C of that issue; Tracks A (BL-24, Laplace reclassification) and B (Laplace recalibration) remain open — the document is correctly scoped to Track C only. Verified — CI on Not independently re-verified: the exact holdout accuracy figures (85.25%, 36.25%, 13.40% etc.) and the sweep tables rely on an uncommitted experiment script. The methodology is transparent (the document describes it fully, including why the script isn't committed — it hits All checkable claims held up. The scope is tight (one manifest value, one BL entry, one audit document), the reasoning is sound, and Finding 2 is correctly handled as a separate concern rather than an untested change. Looks good to merge. |
Merge proposalHead at review:
|
| Key | Value |
|---|---|
| Change | +1/-1 line: dp.parameters.clipping_norm raised from 1.0 to 15.0 |
| Pending proposal | None |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
Documentation/technical/audits/dp-clipping-norm-verification.md
| Key | Value |
|---|---|
| Change | +79 lines (new file): full verification methodology — trained-weight norm sweep, clipping-norm sweep, noise_multiplier sweep, Finding 2 (noise_multiplier independently destructive, unfixed), reproduce instructions |
| Pending proposal | None |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
Developer/BACK_LOG.md
| Key | Value |
|---|---|
| Change | +1/-1 line: BL-23 status updated from 🆕 New to ✅ Resolved; resolved note cross-links the new audit document and records Finding 2 as an open follow-on |
| Pending proposal | None |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
robertocarlous
left a comment
There was a problem hiding this comment.
Verification and merge proposal in the comments above. All checks pass — approving.
|
Thanks, Robbert. Before merging, could we resolve the coordination point in my earlier comment? #263 sets the clipping default to 10, while this PR sets it to 15. That overlap is still pending, so “Pending proposal: None” doesn’t reflect the current discussion. My suggestion remains to keep #263 as the existing scoped change, then assess any further adjustment with reproducible evidence. This isn’t a claim that 15 is incorrect. Since the accuracy figures and sweeps haven’t been independently reproduced, please include the experiment script and exact settings, or narrow the empirical claims. Also, Abraham removed Track C from #279 because the clipping-default work was already covered by #247/#263. Let’s update that scope reference and agree how the two PRs fit together before either lands. |
|
Hi @Abidoyesimze — three small changes needed before this can merge, to resolve the coordination point Santiagocetran raised. All in the existing commit; no new files. 1. PR No. 263 already has the scoped manifest change (raising it to "clipping_norm": 1.0,2. a) Line 3 — remove the "Track C" scope reference. Abraham removed Track C from issue No. 279 (the clipping-default work was already covered by PR No. 247 / PR No. 263). Replace: with: b) Recommendation section — don't claim this PR raised the manifest value. Replace the current Recommendation paragraph and table outro: ...table... with: ...table (unchanged)... 3. In the BL-23 row, replace: with: (Same text, just: remove "issue No. 279 Track C verified against", and change "raised to Sequencing after these changes: PR No. 263 merges first (sets |
…ights (BL-23) Addresses PR InfiniteZeroFoundation#294/InfiniteZeroFoundation#293 review feedback: PR InfiniteZeroFoundation#263 already lands the scoped manifest fix (clipping_norm 1.0 -> 10.0) for BL-23, so this PR no longer touches cache_model_0/manifest.json — it's now just the investigation writeup. Adds Documentation/technical/audits/dp-clipping-norm-verification.md, quantifying how destructive the old clipping_norm: 1.0 default was against the reference model's real trained weights (not the synthetic tensors tests/test_cache_client_dp.py uses) and recommending any value >= 4.0, noting InfiniteZeroFoundation#263's 10.0 already satisfies that with margin. Also documents a related but separate, still-unfixed finding: the default noise_multiplier: 0.5 independently collapses accuracy regardless of clipping_norm, left for a future epsilon/accounting decision rather than fixed here without that context. Updates BL-23's status in Developer/BACK_LOG.md to resolved, crediting PR InfiniteZeroFoundation#263 for the actual default change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4166d9c to
c7c5777
Compare
|
Pushed c7c5777 (rebased onto develop, which now has #263 merged) with the 3 requested changes:
This PR is now just the investigation/audit writeup explaining why |
|
Reviewed against Scope first. Abraham removed BL-23 from issue No. 279 on 26 Oct (comment) because issue No. 247 / PR No. 263 already covered it. PR No. 263 merged on 28 Oct. It raised Claimed: Verified — consistent with Claimed (Finding 2): the default Verified — partly new: PR No. 263 already records the collapse (post-DP accuracy 0.120) and the Claimed ( Verified — accurate, but replaces a short correct cell with a long one: Not independently re-verified: the sweep tables themselves, since the script isn't committed. Recommendation: the part worth keeping is Finding 2. Two options:
Either way, it shouldn't be merged as "Track C of issue No. 279"; that track was removed from the issue before this PR was opened. |
Files changed (2) — as of
|
| Field | Value |
|---|---|
| Change | New |
| Lines | +79/-0 |
| Diff (what exactly is in this PR) | A write-up of a parallel BL-23 experiment: the clipping_norm sweep (Finding 1) and the noise_multiplier sweep plus the √D noise-norm argument (Finding 2). The experiment script isn't committed. |
| Functionality — how & why | How: docs only. Why: to record the measurements behind BL-23. PR No. 263 already landed the fix and reproducible tests for Finding 1, so the new part is Finding 2. |
Diff vs current develop HEAD |
New file; there's no develop counterpart. develop's tests pin different numbers (norm ≈ 9.44, delta ≈ 3.62) from a different training config. |
| Recommended merge proposal | Needs a fix before merge: commit the sweep script (or an integration-marked test), cite PR No. 263's pinned numbers next to these and explain the config difference, and retitle the PR as docs(dp). Alternative: close it and move Finding 2 to issue No. 279. |
| Actual merge proposal | Soon |
| Pending proposal | Author's choice between the two options. |
| Local merge conflict | No |
| GitHub merge conflict | No |
Developer/BACK_LOG.md
| Field | Value |
|---|---|
| Change | Modified |
| Lines | +1/-1 |
| Diff (what exactly is in this PR) | BL-23 status: "✅ Fixed — PR No. 263" → a long "✅ Resolved — verified against a real trained state_dict …" paragraph linking the new doc. |
| Functionality — how & why | How: status cell only. Why: to point BL-23 at the write-up. |
Diff vs current develop HEAD |
Untouched by develop since merge-base. |
| Recommended merge proposal | Drop this change and keep develop's cell. At most, append a link to the doc if it merges. |
| Actual merge proposal | Soon |
| Pending proposal | Revert to develop's cell. |
| Local merge conflict | No |
| GitHub merge conflict | No |
Verification
Docs-only. CI Docs is green on c7c5777. The claims were checked against PR No. 263's merged tests and docs; details are in the verification comment above.
Local vs. GitHub agree: clean on both.
umeradl
left a comment
There was a problem hiding this comment.
Changes requested at c7c5777. Details are in the verification comment.
BL-23 was already fixed by PR No. 263 (merged, with reproducible real-weight tests) and was removed from issue No. 279 before this PR was opened. The new value here is Finding 2, the noise_multiplier sweep. Either:
- commit the sweep script (under
scripts/, or as anintegration-marked test), cite PR No. 263's pinned numbers and the training-config difference, retitle the PR asdocs(dp), and leave BL-23's status cell as it is ondevelop; or - close this PR and post Finding 2 as a comment on issue No. 279.
|
Reviewed against CI on Two files changed vs. Claimed — Claimed — Claimed — BL-23 resolution credits PR No. 263 for the default change and drops "Track C". Verified: the new BL-23 status reads "✅ Resolved — verified against a real trained Claimed — Finding 1 (clipping_norm: 1.0 destructive) is quantitatively correct. The document reports the real global L2 norm at 9.3–12.6 across 5 seeds × 3 configs (consistent range, not a single-seed figure). The sweep table shows utility flat from ~4.0 onward, consistent with 10.0 being safely in the flat region. The √D scaling underpinning Finding 2: Claimed — Finding 2 ( Claimed — doc links resolve. CI Docs check passes (566 links) ✓. Internal link Merge status. Local No findings. Merge-ready pending |
|
Head at review:
|
| Key | Value |
|---|---|
| Change | +79 lines (new file): audit of clipping_norm against real trained state_dict. Finding 1: clipping_norm: 1.0 reduced accuracy 85.25% → 36.25% from clipping alone; real global L2 norm 9.3–12.6 across seeds/configs; flat recovery at ≥4.0; PR No. 263's 10.0 verified in flat region. Finding 2 (new): noise_multiplier: 0.5 collapses accuracy to ~13% at every clipping norm due to √D ≈ 330 scaling; left unfixed pending epsilon/accounting work. |
| Pending proposal | None |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
Developer/BACK_LOG.md
| Key | Value |
|---|---|
| Change | +1/-1 line: BL-23 status updated to "✅ Resolved — verified against a real trained state_dict"; credits PR No. 263 for the clipping_norm: 10.0 default; records Finding 2 (noise_multiplier) as a concrete measured finding; drops "Track C" phrasing; links to the new audit doc |
| Pending proposal | None |
| Recommended merge proposal | Merge as-is |
| Actual merge proposal | Soon |
robertocarlous
left a comment
There was a problem hiding this comment.
Docs and BACK_LOG only — clipping_norm verification doc internally consistent, √D ≈ 330 maths checks out, no manifest delta vs develop, BL-23 correctly credits PR No. 263, no Track C language. Verified at c7c5777
… follow-on) BL-23's clipping_norm question is already resolved on develop by PR InfiniteZeroFoundation#263, which raised the manifest default to 10.0 and added reproducible tests pinning the reference model's real post-training L2 norm (~9.44) and one-round update_gaussian delta norm (~3.62). This records a related, previously unscoped finding surfaced while cross-checking that work: at the manifest's default noise_multiplier (0.5), Gaussian DP noise collapses model utility to the random-guessing floor regardless of clipping_norm, because the noise's aggregate L2 norm scales with noise_multiplier * clipping_norm * sqrt(num_parameters), not just noise_multiplier * clipping_norm. scripts/dp_noise_multiplier_sweep.py reproduces this using the exact same fixture as PR InfiniteZeroFoundation#263's _build_and_train_reference() (the committed cache_model_0/dataset/test/test_dataset.pt, seed=42, 2000 samples, 20 epochs, lr=0.01) -- its printed post-training/delta norms match PR InfiniteZeroFoundation#263's pinned values exactly, confirming it's the same fixture, and it needs no network access. Finding 1 (the clipping_norm question) is left as PR InfiniteZeroFoundation#263's resolution; this doc's own exploratory clipping_norm sweep (a different, uncommitted config -- 400 samples/10 epochs/Adam lr=0.001, train_client_model's actual hyperparameters) is kept only as corroborating evidence, explicitly labelled as using a different config from PR InfiniteZeroFoundation#263's pinned numbers. BL-23's status cell on develop (set by PR InfiniteZeroFoundation#263) is left untouched. Renamed from dp-clipping-norm-verification.md to dp-noise-multiplier-verification.md to match the finding this doc now centers on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
c7c5777 to
082fd4c
Compare
|
Pushed 082fd4c, going with option 1:
Rebased onto current |
Summary
BL-23's
clipping_normquestion is already resolved ondevelopby PR #263, which raised the manifest default to10.0and added reproducible tests (_build_and_train_reference) pinning the reference model's real post-training L2 norm (~9.44) and one-roundupdate_gaussiandelta norm (~3.62).This PR records a related, previously unscoped finding surfaced while cross-checking that work: at the manifest's default
noise_multiplier: 0.5, Gaussian DP noise collapses model utility to the random-guessing floor regardless ofclipping_norm, because the noise's aggregate L2 norm scales withnoise_multiplier * clipping_norm * √(num_parameters), not justnoise_multiplier * clipping_norm.Changes
scripts/dp_noise_multiplier_sweep.py: reproduces the finding using the exact same fixture as PR fix(dp): retune clipping_norm for post_training mechanisms and add real-weight tests (BL-23) #263's_build_and_train_reference()(the committedcache_model_0/dataset/test/test_dataset.pt, seed=42, 2000 samples, 20 epochs, lr=0.01). Its printed post-training/delta norms (9.44 / 3.62) match PR fix(dp): retune clipping_norm for post_training mechanisms and add real-weight tests (BL-23) #263's pinned values exactly, confirming it's the same fixture — no network access needed, same techniquetests/test_cache_client_dp.py's_load_test_dataset()uses to avoid atorchvisiondependency.Documentation/technical/audits/dp-clipping-norm-verification.md→dp-noise-multiplier-verification.mdto center on this finding. Theclipping_normquestion (Finding 1) is left as PR fix(dp): retune clipping_norm for post_training mechanisms and add real-weight tests (BL-23) #263's resolution; this doc's own earlier, separate exploratoryclipping_normsweep (different config — 400 samples/10 epochs/Adam lr=0.001, matchingtrain_client_model's real hyperparameters) is kept only as corroborating evidence, explicitly labelled as a different, non-reproduced config from PR fix(dp): retune clipping_norm for post_training mechanisms and add real-weight tests (BL-23) #263's pinned numbers.Developer/BACK_LOG.md's BL-23 status cell is not touched — left as PR fix(dp): retune clipping_norm for post_training mechanisms and add real-weight tests (BL-23) #263 set it..gitignore: added!scripts/dp_noise_multiplier_sweep.py(the directory is otherwise ignored except for the existingpreflight.py).Test plan
python3 scripts/dp_noise_multiplier_sweep.py— prints post-training norm 9.44, delta norm 3.62 (matches PR fix(dp): retune clipping_norm for post_training mechanisms and add real-weight tests (BL-23) #263's pinned values), and thenoise_multipliersweep: 10.50% accuracy at the default0.5, recovering to ~85.5% only oncenoise_multiplierdrops to0.001.pytest tests/test_cache_client_dp.py -q— 31 passed, unaffected.python3 .github/scripts/check_doc_links.py Documentation Developer— 573 links, all resolve.Addresses review feedback on prior revisions of this PR — see the PR comments for the full discussion with @Santiagocetran, @robertocarlous, and @umeradl.
🤖 Generated with Claude Code