Give the cond_cat/cond_cont column layout one owner (gitea #37) #55

Merged

lars merged 1 commits from fix/issue-37 into master 2026-08-14 14:24:52 +02:00

Conversation 0 Commits 1 Files Changed 9

+425 -151

lars commented 2026-08-14 14:18:12 +02:00

Owner

Copy Link

Copy Source

The conditioning arrays' column order was written down three times — twice
in giant/data/transforms.py (build_cond_features and build_features each
built cond_cont and cond_cat from scratch) and again in
giant/model/encoders.py (cat_col_layout, plus hand-written
COND_DIM_BASE + PARTICLE_PHYS_DIM slicing in ConditionEncoder). The three
were held in sync only by parallel comments, so a wrong column order
produced silently mis-indexed features rather than an exception.

The drift had already happened, twice, both times in build_features:

• 5b63dfd added per-axis vocab-lookup strictness (an out-of-vocab
  pdg/material must not KeyError under "physical"/"onehot", where the
  index is never read) to build_cond_features only.
• _cond_normalizer_transform's legacy-normalizer padding, which keeps a
  pre-physical-conditioning 8-wide cond normalizer loadable, was likewise
  only wired into build_cond_features — so giant predict on such a
  checkpoint died with a broadcast error.

New giant/cond_layout.py holds a frozen CondLayout built from the
(particle, material) mode pair, exposing named cond_cont slices
(base/particle_phys/material_phys) and cond_cat columns
(PDG_COL/MAT_COL/particle_topn_col/material_topn_col/cat_dim). Both
builders now share one _build_cond_arrays, ConditionEncoder reads its
slices off the same object, and PdgRouter/ProcessRouter use the named
dense-vocab columns instead of literal 0/1. CondLayout also absorbs the
two duplicated axis-type validations, keeping their message text verbatim.

Decisions taken while planning:

• Scope is CondLayout only. The issue's second half — a
  CONDITIONING_AXIS_REGISTRY registering (feature_columns, encoder_module)
  as a pair — is deferred: it would force ConditioningConfig's fixed
  particle/material fields into a dynamic axis map and ripple through
  pipeline.py, checkpoint_io.py and rollout.py, i.e. a config-schema break
  with no consumer yet.
• The two divergences above are unified onto build_cond_features'
  behaviour rather than preserved as parameters, so the new single source
  of truth doesn't carry the old split forward. Each gets a regression
  test that fails before this commit.
• cat_col_layout is replaced outright (deleted, dropped from network.py's
  all, its four tests rewritten against CondLayout) rather than kept
  as a wrapper — two spellings of the same fact is the defect itself.

cond_cat's width is now the layout's call rather than "did the caller pass
a map", so an "onehot" axis without its top-N map raises instead of
yielding a narrower array that ConditionEncoder would index out of bounds.
pipeline.py's normalizer-fitting pass reads only cond_cont but had to be
handed the maps to satisfy that.

No parameter, buffer or state_dict change; existing checkpoints load
unchanged, and the protected migration surfaces are untouched.

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

The conditioning arrays' column order was written down three times — twice in giant/data/transforms.py (build_cond_features and build_features each built cond_cont and cond_cat from scratch) and again in giant/model/encoders.py (cat_col_layout, plus hand-written COND_DIM_BASE + PARTICLE_PHYS_DIM slicing in ConditionEncoder). The three were held in sync only by parallel comments, so a wrong column order produced silently mis-indexed features rather than an exception. The drift had already happened, twice, both times in build_features: - 5b63dfd added per-axis vocab-lookup strictness (an out-of-vocab pdg/material must not KeyError under "physical"/"onehot", where the index is never read) to build_cond_features only. - _cond_normalizer_transform's legacy-normalizer padding, which keeps a pre-physical-conditioning 8-wide cond normalizer loadable, was likewise only wired into build_cond_features — so `giant predict` on such a checkpoint died with a broadcast error. New giant/cond_layout.py holds a frozen CondLayout built from the (particle, material) mode pair, exposing named cond_cont slices (base/particle_phys/material_phys) and cond_cat columns (PDG_COL/MAT_COL/particle_topn_col/material_topn_col/cat_dim). Both builders now share one _build_cond_arrays, ConditionEncoder reads its slices off the same object, and PdgRouter/ProcessRouter use the named dense-vocab columns instead of literal 0/1. CondLayout also absorbs the two duplicated axis-type validations, keeping their message text verbatim. Decisions taken while planning: - Scope is CondLayout only. The issue's second half — a CONDITIONING_AXIS_REGISTRY registering (feature_columns, encoder_module) as a pair — is deferred: it would force ConditioningConfig's fixed particle/material fields into a dynamic axis map and ripple through pipeline.py, checkpoint_io.py and rollout.py, i.e. a config-schema break with no consumer yet. - The two divergences above are unified onto build_cond_features' behaviour rather than preserved as parameters, so the new single source of truth doesn't carry the old split forward. Each gets a regression test that fails before this commit. - cat_col_layout is replaced outright (deleted, dropped from network.py's __all__, its four tests rewritten against CondLayout) rather than kept as a wrapper — two spellings of the same fact is the defect itself. cond_cat's width is now the layout's call rather than "did the caller pass a map", so an "onehot" axis without its top-N map raises instead of yielding a narrower array that ConditionEncoder would index out of bounds. pipeline.py's normalizer-fitting pass reads only cond_cont but had to be handed the maps to satisfy that. No parameter, buffer or state_dict change; existing checkpoints load unchanged, and the protected migration surfaces are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

lars added 1 commit 2026-08-14 14:18:13 +02:00

Give the cond_cat/cond_cont column layout one owner (gitea #37) ... 4692cee699

The conditioning arrays' column order was written down three times — twice
in giant/data/transforms.py (build_cond_features and build_features each
built cond_cont and cond_cat from scratch) and again in
giant/model/encoders.py (cat_col_layout, plus hand-written
COND_DIM_BASE + PARTICLE_PHYS_DIM slicing in ConditionEncoder). The three
were held in sync only by parallel comments, so a wrong column order
produced silently mis-indexed features rather than an exception.

The drift had already happened, twice, both times in build_features:

- 5b63dfd added per-axis vocab-lookup strictness (an out-of-vocab
  pdg/material must not KeyError under "physical"/"onehot", where the
  index is never read) to build_cond_features only.
- _cond_normalizer_transform's legacy-normalizer padding, which keeps a
  pre-physical-conditioning 8-wide cond normalizer loadable, was likewise
  only wired into build_cond_features — so `giant predict` on such a
  checkpoint died with a broadcast error.

New giant/cond_layout.py holds a frozen CondLayout built from the
(particle, material) mode pair, exposing named cond_cont slices
(base/particle_phys/material_phys) and cond_cat columns
(PDG_COL/MAT_COL/particle_topn_col/material_topn_col/cat_dim). Both
builders now share one _build_cond_arrays, ConditionEncoder reads its
slices off the same object, and PdgRouter/ProcessRouter use the named
dense-vocab columns instead of literal 0/1. CondLayout also absorbs the
two duplicated axis-type validations, keeping their message text verbatim.

Decisions taken while planning:

- Scope is CondLayout only. The issue's second half — a
  CONDITIONING_AXIS_REGISTRY registering (feature_columns, encoder_module)
  as a pair — is deferred: it would force ConditioningConfig's fixed
  particle/material fields into a dynamic axis map and ripple through
  pipeline.py, checkpoint_io.py and rollout.py, i.e. a config-schema break
  with no consumer yet.
- The two divergences above are unified onto build_cond_features'
  behaviour rather than preserved as parameters, so the new single source
  of truth doesn't carry the old split forward. Each gets a regression
  test that fails before this commit.
- cat_col_layout is replaced outright (deleted, dropped from network.py's
  __all__, its four tests rewritten against CondLayout) rather than kept
  as a wrapper — two spellings of the same fact is the defect itself.

cond_cat's width is now the layout's call rather than "did the caller pass
a map", so an "onehot" axis without its top-N map raises instead of
yielding a narrower array that ConditionEncoder would index out of bounds.
pipeline.py's normalizer-fitting pass reads only cond_cont but had to be
handed the maps to satisfy that.

No parameter, buffer or state_dict change; existing checkpoints load
unchanged, and the protected migration surfaces are untouched.

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

lars merged commit c71210f006 into master 2026-08-14 14:24:52 +02:00

lars deleted branch fix/issue-37 2026-08-14 14:24:55 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

architecture

bug

Silent/wrong behavior, no crash

experiment

feature

New feature matching the v0.3 redesign

investigation

modularity

Making the network genuinely pluggable

performance

phase2

validation

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

lars (Lars Bogner)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lars/giant#55