Skip to content

fix: flux defaults to KL per eq 7; unify four drifted defaults - #47

Merged
nceglia merged 1 commit into
mainfrom
fix/defaults-and-flux
Aug 6, 2026
Merged

fix: flux defaults to KL per eq 7; unify four drifted defaults#47
nceglia merged 1 commit into
mainfrom
fix/defaults-and-flux

Conversation

@nceglia

@nceglia nceglia commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Two small fixes and one new guard.

1. phenotypic_flux defaults to KL

METRICS eq 7 defines phenotypic flux as the KL divergence. The code defaulted to
l1. Now distance_metric="kl"; l1 and jsd remain available as extensions.

⚠️ This changes reported flux numbers for any caller relying on the default. The
kernels already existed in tcri/_distance.py — only the default moved. Retires
OPEN_QUESTIONS['flux_distance_default'].

2. Four defaults declared twice, with different values

A knob declared in both layers means the outer declaration silently wins and the inner
one is dead — reachable only by constructing TCRIModule or UnifiedTrainingPlan
directly, which is exactly what a fixture or downstream user does.

knob outer inner effective now
global_scale (α) TCRIModel 5.0 TCRIModule 10.0 5.0 5.0
local_scale (β) TCRIModel 3.0 TCRIModule 5.0 3.0 3.0
n_steps_kl_warmup train() 2000 UnifiedTrainingPlan 1000 2000 2000
reconstruction_loss_scale 1e-3 unified earlier in 19db68e

Each is unified to the value that was already effective, so no fitted result
changes
. This is not a choice about the right hyperparameter — it removes a value that
silently does nothing.

3. tests/test_shared_defaults.py — guards the class, not the instances

Compares every shared default across the two construction seams
(TCRIModel.__init__TCRIModule.__init__, TCRIModel.train
UnifiedTrainingPlan), plus a regression lock on the four known ones.

global_scale was found by this test on its first run, having been missed by every
manual pass including the one earlier today that turned up the other three.

The existing knob test could not catch this: it verifies a value arrives at its target,
which it does. The defect is that the two declared defaults differ, so anyone reading
one signature is misled about what the package does. Signature-only — no model is
constructed, runs in ~3 s.

Tests

177 passed, 3 skipped.

🤖 Generated with Claude Code

flux: distance_metric now defaults to 'kl'. METRICS eq 7 defines phenotypic flux AS the
KL divergence; l1/jsd stay available as extensions. This CHANGES reported flux numbers for
callers relying on the default. Retires OPEN_QUESTIONS['flux_distance_default'].

defaults: four knobs were declared in two layers with different values. The outer
declaration silently wins, so the inner one is dead code that only bites someone
constructing TCRIModule or UnifiedTrainingPlan directly. Unified each to the value that was
already effective, so no fitted result changes:

  global_scale        TCRIModel 5.0  vs TCRIModule 10.0    -> 5.0
  local_scale         TCRIModel 3.0  vs TCRIModule 5.0     -> 3.0
  n_steps_kl_warmup   train()  2000  vs UnifiedTrainingPlan 1000 -> 2000
  (reconstruction_loss_scale was the same defect, unified earlier in 19db68e)

global_scale was found by the new test on its first run, having been missed by every
manual pass including today's.

tests/test_shared_defaults.py guards the class rather than the instances: it compares every
shared default across the two construction seams. The knob test could not catch this -- it
verifies a value ARRIVES at its target, which it does; the defect is that the two declared
defaults differ.

177 passed, 3 skipped.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 5, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@nceglia
nceglia merged commit 46490e6 into main Aug 6, 2026
2 checks passed
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