Skip to content
This repository was archived by the owner on Aug 20, 2026. It is now read-only.
This repository was archived by the owner on Aug 20, 2026. It is now read-only.

bug: field_matvec_rls's JRLS (wc==nc) path silently ignores per-channel weight variation #65

Description

@balbasty

Summary

Found while root-causing #62 (fixed by
fastfields-cpu-impl#48 / #64). This is a second, distinct bug in
the same code region, currently not caught by any test, and
not fixed by #64.

field_matvec_rls/field_diag_rls/field_relax_rls dispatch to a
"_jrls" family of functions when the weight tensor's trailing
dimension equals the field's channel count (wc == nc) — the
genuine "one weight value per field channel" case, per the doc
comment in fastfields-cpu-lib/reg_field.cpp:

is_jrls selects between the single-shared-weight (RLS) and
per-channel-weight (JRLS) impl variants

In practice, the _jrls kernels (matvec_absolute_jrls,
matvec_membrane_jrls, matvec_bending_jrls, and their diag_*
counterparts, in fastfields-kernels/regularisers/field/{1d,2d,3d}.h)
read the weight value once (w = *wgt / w11 = *wgt / w22 = *wgt, outside any per-channel loop) and apply that single value to
every field channel. They have no per-channel stride parameter
at all — contrast with the (now-fixed) _rls family, whose
wgt[wsc*c]-style indexing is exactly the per-channel read the
_jrls family is missing.

At the fastfields-cpu-impl layer
(fastfields-cpu-impl/reg_field.h), the driver functions for
matvec_membrane_jrls etc. mostly don't even compute a channel
stride for wgt at all; matvec_membrane_jrls is the one exception
— it computes wsc = stride_wgt[nall] but never passes it to the
kernel call, i.e. it's dead code (a leftover from an unfinished
wiring attempt, and itself a hint that this was noticed once before
and not finished).

Net effect: for a genuine per-channel weight map (wc == nc,
e.g. different reliability weights per field channel), every channel
silently gets channel 0's weight instead of its own. field_diag_rls
and field_relax_rls inherit the same problem since they share the
kernel-level weight read.

Why current tests don't catch it

  • run_2d_matvec_rls_unit_weight (added in test: check the RLS field operator reduces to the unweighted one at w == 1 #59) uses an all-ones
    weight map for both wc=1 and wc=nc — channel 0's weight equals
    every other channel's weight (all 1.0), so the bug is invisible.
  • run_2d_matvec_rls_symmetry only checks self-adjointness, which
    holds regardless of which weight value gets broadcast, as long as
    it's applied consistently forward and adjoint.
  • run_2d_diag_rls only checks diag against matvec on unit
    vectors through the same (buggy) weight-broadcast path — a
    self-consistency check, not a ground-truth one.

None of the existing oracles construct a wc == nc weight map with
genuinely different values per channel and check the result actually
varies by channel.

Suggested fix

Give the _jrls kernels a real per-channel stride (mirroring what
the _rls family already does, now that #64 correctly zeroes it for
the broadcast case) and add a genuine ground-truth regression test:
wc == nc weights that differ per channel, checked against a
brute-force reference that actually varies the applied weight by
channel. This spans fastfields-kernels (six kernel functions ×
three dimensionality files: matvec/diag × absolute/membrane/
bending × 1d/2d/3d), fastfields-cpu-impl (nine driver
functions), and a new cpu-lib test.

Scope / impact

part of the field_matvec_rls/#62 investigation.

Workstream: claude-jitfields-to-fastfields

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions