Skip to content

Trim AlgorithmsInterfaceExtensions to a minimal NestedAlgorithm#115

Open
mtfishman wants to merge 15 commits into
mainfrom
mf/nested-algorithm
Open

Trim AlgorithmsInterfaceExtensions to a minimal NestedAlgorithm#115
mtfishman wants to merge 15 commits into
mainfrom
mf/nested-algorithm

Conversation

@mtfishman
Copy link
Copy Markdown
Member

@mtfishman mtfishman commented May 19, 2026

Trim AlgorithmsInterfaceExtensions to just NestedAlgorithm — abstract type + initialize_subsolve + finalize_substate! + AI.step!. Everything else (Problem/Algorithm/State abstract types, DefaultState, DefaultNestedAlgorithm, FlattenedAlgorithm, AlgorithmIterator, NonIterativeAlgorithm, …) is removed, along with the placeholder src/sweeping/ and its tests.

The belief-propagation code is refactored into three Problem / Algorithm / State triples — outer BeliefPropagation*, middle BeliefPropagationSweep*, inner MessageUpdate* — each subtyping AI.* directly and modeled on the WIP gate-apply code (#114).

…ubstate!`

Rename the `NestedAlgorithm` step hooks from `get_subproblem` /
`set_substate!` to `initialize_subsolve` / `finalize_substate!`, and move
the indexed-list dispatch (`algorithm.algorithms[state.iteration]`) out
of the abstract type's default into `DefaultNestedAlgorithm`'s own
`initialize_subsolve` method. The abstract `NestedAlgorithm` now has no
default `initialize_subsolve` impl (throws `MethodError`), so subtypes
must provide their own — the previous default silently assumed every
subtype carried an `algorithms` vector, which is too narrow.

`BeliefPropagation` and `BeliefPropagationSweep` get an explicit
`AIE.initialize_subsolve` override mirroring the indexed-list shape;
the existing `AIE.set_substate!` override on `BeliefPropagationSweep`
is renamed to `AIE.finalize_substate!`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
mtfishman added a commit that referenced this pull request May 19, 2026
Sync the apply-PR's local `NestedAlgorithm` definition with the rename
landing in #115. Once #115 merges, this local definition will be
removed entirely in favor of `AIE.NestedAlgorithm`.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 77.69231% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.23%. Comparing base (bd91366) to head (3c9c2d8).

Files with missing lines Patch % Lines
...terfaceExtensions/AlgorithmsInterfaceExtensions.jl 66.07% 19 Missing ⚠️
src/beliefpropagation/beliefpropagation.jl 86.48% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
- Coverage   75.81%   73.23%   -2.58%     
==========================================
  Files          23       21       -2     
  Lines         980      923      -57     
==========================================
- Hits          743      676      -67     
- Misses        237      247      +10     
Flag Coverage Δ
docs 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Delete from AIE:
- `DefaultNestedAlgorithm` (struct, constructor, and its
  `initialize_subsolve` indexed-list impl).
- `nested_algorithm` factory function + `max_iterations`.
- All `FlattenedAlgorithm` machinery (struct, state, helper).
- `AlgorithmIterator` / `DefaultAlgorithmIterator` /
  `algorithm_iterator`.
- `with_algorithmlogger`.
- `NonIterativeAlgorithm` / `DefaultNonIterativeAlgorithmState`.

Keep the minimum BP / nested-solve still needs: `Problem`, `Algorithm`,
`State`, `DefaultState`, `AI.initialize_state` / `initialize_state!` /
`increment!`, and the minimal `NestedAlgorithm` (abstract type +
`initialize_subsolve` + `finalize_substate!` + `AI.step!`). Future
features can be added back as concrete subtypes when actually needed.

Delete the placeholder sweep / DMRG scaffolding (`src/sweeping/`,
`test/test_dmrg.jl`, `test/test_sweeping.jl`): `EigsolveRegion` /
`EigenProblem` / `dmrg` / `select_algorithm` were not actually wired up
(the `solve!` body threw `not implemented yet`), and the tests covered
only construction shape. Inline the kwarg-expansion helpers
(`extend_columns`, `rows`, ...) that lived in `sweeping/utils.jl` into
`beliefpropagation/beliefpropagationproblem.jl` — its only remaining
consumer.

Prune `test/test_algorithmsinterfaceextensions.jl` to the surface that
still exists; add a `TestNestedAlgorithm` concrete subtype mirroring how
`BeliefPropagation` shapes itself on top of the new minimal interface,
and a test that the bare `initialize_subsolve` default throws.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@mtfishman mtfishman changed the title Refactor NestedAlgorithm hooks: initialize_subsolve + finalize_substate! Trim AlgorithmsInterfaceExtensions to a minimal NestedAlgorithm May 19, 2026
mtfishman and others added 4 commits May 19, 2026 16:19
`AlgorithmsInterfaceExtensions` is now just the `NestedAlgorithm`
abstract type plus `initialize_subsolve` / `finalize_substate!` /
`AI.step!`. The `Problem` / `Algorithm` / `State` abstract types and
`DefaultState` are gone, along with the `AI.initialize_state` /
`initialize_state!` / `increment!` overloads that hung off them.

Belief propagation grows its own state machinery instead of leaning on
AIE: a `BeliefPropagationState <: AI.State` (mutable, `iterate` /
`iteration` / `stopping_criterion_state`), plus per-algorithm
`AI.initialize_state` / `initialize_state!` / `increment!` overloads on
`Union{BeliefPropagation, BeliefPropagationSweep}`. This mirrors the
pattern `BPApplyGate` already uses in the apply-operator path — each
algorithm owns its state, no shared default.

`BeliefPropagationProblem`, `BeliefPropagation`, and
`BeliefPropagationSweep` now subtype the bare `AI.Problem` /
`AI.Algorithm` types; the `StopWhenConverged` dispatches accept
`AI.Problem` / `AI.Algorithm` / `AI.State` since `StopWhenConverged`
itself is the unique type doing the disambiguation.

`test_algorithmsinterfaceextensions.jl` is rewritten to define its
own `TestProblem` / `TestChildAlgorithm` / `TestChildState` /
`TestNestedAlgorithm` directly on `AI.*`, so the test exercises the
same "each algorithm owns its state" pattern.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Layer 1 — outer BP loop:
  `BeliefPropagationProblem` / `BeliefPropagationAlgorithm` /
  `BeliefPropagationState` (iterative, `<: AIE.NestedAlgorithm`)

Layer 2 — one sweep over edges:
  `BeliefPropagationSweepProblem` / `BeliefPropagationSweepAlgorithm` /
  `BeliefPropagationSweepState` (iterative, `<: AIE.NestedAlgorithm`)

Layer 3 — single-edge message update:
  `MessageUpdateProblem` / `SimpleMessageUpdateAlgorithm` /
  `MessageUpdateState` (non-iterative, overrides `AI.solve_loop!` —
  same shape `BPApplyGate` uses in the gate-apply code)

Every layer subtypes `AI.Problem` / `AI.Algorithm` / `AI.State`
directly. No `Union{...}` shortcuts — each layer's `initialize_state` /
`initialize_state!` / `initialize_subsolve` is its own method. The
`StopWhenConverged` dispatches accept `AI.Problem` / `AI.Algorithm` /
`AI.State` since `StopWhenConverged` itself is the unique
disambiguating type.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
API first, then the three layers from outer (BP) to inner (message
update), then the supporting `StopWhenConverged` stopping criterion +
`iterate_diff` helper, then the low-level kwarg utilities at the bottom.
Reads as "what's the API → how it's composed → supporting pieces" the
way the file is most likely to be skimmed.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`StopWhenConverged` is fully generic — its dispatches accept
`AI.Problem` / `AI.Algorithm` / `AI.State`, with `StopWhenConverged`
itself as the unique disambiguating type. The only piece that needed
to live in BP was the metric: the BP `iterate_diff(::MessageCache,
::MessageCache)` override.

AIE now owns: `StopWhenConverged`, `StopWhenConvergedState`, the four
`AI.*` overloads, and a bare `iterate_diff(a, b)` verb that throws by
default. BP provides its concrete `AIE.iterate_diff(::MessageCache,
::MessageCache)`. The top-level `ITensorNetworksNext` namespace
re-exports `StopWhenConverged` + `iterate_diff` for callers that use
the package-qualified names.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Comment on lines +125 to +127
struct BeliefPropagationSweepProblem{Factors} <: AI.Problem
factors::Factors
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was anticipating this would also take in the edge order to be updated, as that defines the sweep problem (we wish to sweep over these edges of the factor graph). Something like:

# Problem of solving this sequence of region sweeps on this factor graph
struct BeliefPropagationSweepProblem{Factors, Regions} <: AI.Problem
    factors::Factors
    regions::Regions
end

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed, good point.

Copy link
Copy Markdown
Contributor

@jack-dunham jack-dunham May 19, 2026

Choose a reason for hiding this comment

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

One can then directly use his problem:

struct BeliefPropagationSweepProblem{Factors, Edges} <: AI.Problem
    factors::Factors
    edges::Edges
end

to implement tree BP, or in combination with a message update algorithm that gauges to implement a gauge walk. e.g:

struct TreeBeliefPropagationProblem{Factors, V} <: AI.Problem
    factors::Factors
    root::V
end
function solve!(prob::TreeBeliefPropagationProblem, alg::TreeBeliefPropagationAlgorithm, state)
    edges = ... # use `alg` and `prob.root` to make edge sequence
    sweep = BeliefPropagationSweepProblem(prob.factors, edges)
    solve!(sweep, ...)
end

Comment on lines 189 to 191
struct MessageUpdateProblem{Factors} <: AI.Problem
factors::Factors
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again, I envisioned:

# Update the message associated with `region` of the factor graph defined by `factors`.
struct MessageUpdateProblem{Factors, Region} <: AI.Problem
    factors::Factors
    region::Region
end

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, this was the part of the design I was least sure about, i.e. which information should be the problem, algorithm, and state for the message update. I think that makes sense to put which edge is being updated in the problem. Is there a reason to call it "region" instead of "edge"? I would find "edge" easier to understand.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Edge is fine, I was just trying to be generic.

Comment on lines 201 to 203
@kwdef mutable struct MessageUpdateState{Iterate} <: AI.State
iterate::Iterate
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this need to be a subtype of AI.State? Can one not just use the MessageCache directly? Is it because we need to dispatch on solve_loop! now instead of solve!?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, it is because we are using parts of the AlgorithmsInterface like solve!, intialize_state, etc. I agree that would be less code/boilerplate, for this layer it is a lot of boilerplate for something pretty simple, we could definitely debate about which level we should overload. I found this choice to be conceptually a little bit simpler.

…teProblem`

The edge is per-step data; it belongs on the problem side, not the
algorithm side. After the move:

- `MessageUpdateProblem{Factors, Edge <: AbstractEdge}` carries
  `factors` + `edge` (type parameter renamed from `E` to `Edge`).
- `SimpleMessageUpdateAlgorithm{ContractionAlg}` is now just
  `normalize` + `contraction_alg` — no per-edge data.
- `BeliefPropagationSweepProblem` gains an `edges` field; its
  `initialize_subsolve` picks `edge = problem.edges[state.iteration]`
  and threads it into the `MessageUpdateProblem`.
- `BeliefPropagationProblem` gains an `edges` field too, and the outer
  `initialize_subsolve` threads it down to the sweep subproblem.
- The entry-point `do edge` closure becomes `do _` since the algorithm
  no longer needs the edge.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jack-dunham
Copy link
Copy Markdown
Contributor

jack-dunham commented May 19, 2026

I was thinking BeliefPropagationSweepAlgorithm could host an ordered mapping from an edge to a message update algorithm. Then one does:

function initialize_subsolve!(problem::BeliefPropagationProblem, algorithm::BeliefPropagationAlgorithm, state::BeliefPropagationState)
    subalgorithm = algorithm.sweeps[state.iteration] # a BeliefPropagationSweepAlgorithm
    edges = keys(subalgorithm.updates)
    subproblem = BeliefPropagationSweepProblem(problem.factors, edges)
    substate = initialize_state(subproblem, subalgorithm; iterate = state.iterate)
    return subproblem, subalgorithm, substate
end

and also (property/variable names for illustration only)

function initialize_subsolve!(problem::BeliefPropagationSweepProblem, algorithm::BeliefPropagationSweepAlgorithm, state::BeliefPropagationSweepState)
    edge = problem.edges[state.iteration]
    subalgorithm = algorithm.updates[edge]
    subproblem = MessageUpdateProblem(problem.factors, edge)
    substate = initialize_state(subproblem, subalgorithm; iterate = state.iterate)
    return subproblem, subalgorithm, substate
end

mtfishman and others added 2 commits May 19, 2026 17:46
…gationSweepAlgorithm`

At the outer BP layer the edge update order is an algorithmic choice
(which edges to sweep and in what order, potentially varying per rep),
not problem data. So edges now live on `BeliefPropagationSweepAlgorithm`
— one sweep algorithm is "do a sweep with these edges". The outer
`initialize_subsolve` transfers them into a `BeliefPropagationSweepProblem`
when stepping down a layer.

Side effect: `BeliefPropagationSweepAlgorithm` no longer needs an
`algorithms::Vector` field. The per-edge `SimpleMessageUpdateAlgorithm`s
were always identical copies (they no longer carry an edge), so it
collapses to a single `message_update_algorithm` template — matching the
shape `ApplyOperators` uses in the gate-apply code.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Edges shouldn't be stored at both the outer BP algorithm level (as the
algorithmic edge-ordering choice) and the sweep algorithm level (as
runtime data that gets copied into the sweep problem). Keep them only
on `BeliefPropagationAlgorithm`; the outer `initialize_subsolve` then
transfers them into the `BeliefPropagationSweepProblem` when stepping
down.

`BeliefPropagationSweepAlgorithm` is now just `message_update_algorithm`
+ `stopping_criterion` (the stopping criterion is sized to the edges at
construction time in `beliefpropagation()`).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@mtfishman
Copy link
Copy Markdown
Member Author

I was thinking BeliefPropagationSweepAlgorithm could host an ordered mapping from an edge to a message update algorithm.

Interesting idea, I'll try to incorporate that into the PR.

mtfishman and others added 6 commits May 19, 2026 18:54
Make `BeliefPropagationSweepAlgorithm.algorithms` indexable by edge
(default: a `Dict{Edge, MessageUpdateAlgorithm}` populated with the same
template at construction time), and drop the
`Algorithms <: AbstractVector{ChildAlgorithm}` constraint from both
`BeliefPropagationAlgorithm` and `BeliefPropagationSweepAlgorithm` so any
indexable container works.

Edges remain sourced from `BeliefPropagationSweepProblem` (the source of
truth); the sweep `initialize_subsolve` looks up
`algorithm.algorithms[edge]` to pick the per-edge update algorithm.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The file now defines three problem/algorithm/state triples plus the
top-level `beliefpropagation` entry point, so the old name is misleading.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Public-API changes:

  * `beliefpropagation` no longer takes a `maxiter` kwarg; pass either
    `stopping_criterion = (; maxiter = 10)` or a full criterion such as
    `stopping_criterion = AI.StopAfterIteration(10) | StopWhenConverged(1.0e-10)`.
  * The `message_update_algorithm` kwarg accepts either a NamedTuple of
    keyword arguments forwarded to `SimpleMessageUpdateAlgorithm`, or a
    full `AI.Algorithm`.
  * The `edges` kwarg defaults to `default_beliefpropagation_edges(factors)`
    rather than being computed inside the body.
  * Per-iteration kwarg broadcasting (`extend_columns`, `rows`,
    `rowlength`, `repeat_last`) is removed.

Internals:

  * `BeliefPropagationSweepAlgorithm` now holds a single
    `message_update_algorithm` (was a `Dict{Edge, ...}` of per-edge
    algorithms). Edge-dependent updates are expressed by defining a new
    `AI.Algorithm` subtype and dispatching on `problem.edge` in
    `solve_loop!`.
  * `BeliefPropagationAlgorithm` holds a single `sweep_algorithm` (was a
    `Vector` of per-iteration sweep algorithms). Iteration-dependent
    sweep behavior is expressed by defining a new sweep algorithm
    subtype and varying the inner algorithm in
    `AIE.initialize_subsolve` using `state.iteration`.
  * New `select_*` selectors `select_beliefpropagation_stopping_criterion`
    and `select_message_update_algorithm`, modeled on
    `MatrixAlgebraKit.select_truncation`, normalize NamedTuple-or-object
    inputs into algorithm/criterion objects.
  * `default_beliefpropagation_edges` and
    `default_message_update_algorithm` expose the defaults as named
    functions.
  * Stopping-criterion default removal: the entry point no longer
    constructs a `StopAfterIteration(maxiter)` itself — choice of
    criterion is entirely the caller's responsibility, except for the
    inner sweep, which still defaults to `StopAfterIteration(length(edges))`.
  * Type parameter `SCState` renamed to `StoppingCriterionState` on both
    BP state structs.
  * Helper definitions reordered so default/select helpers sit above
    `beliefpropagation` for readability.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reorder the body of `beliefpropagation` so the algorithm construction
flows top-to-bottom and `cache` is constructed alongside the
`AI.solve` call, and collapse a few short multi-line forms onto single
lines where the formatter is happy with it.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…m` strategy

The third `AlgorithmsInterface` layer (`MessageUpdateProblem` /
`SimpleMessageUpdateAlgorithm` / `MessageUpdateState`) was a non-iterative
function call dressed in problem/algorithm/state ceremony; this commit
drops that framing. The per-edge update body lives in
`AI.step!(::BeliefPropagationSweepProblem, ::BeliefPropagationSweepAlgorithm,
::BeliefPropagationSweepState)` and delegates to a new strategy interface:

  - `abstract type MessageUpdateAlgorithm end`
  - `message_update!(algorithm, cache, factors, edge)`
  - `SimpleMessageUpdate <: MessageUpdateAlgorithm` (the default;
    holds `normalize` + `contraction_alg`)

`BeliefPropagationSweepAlgorithm` now holds a `message_update_algorithm`
field (default `SimpleMessageUpdate()`) and is no longer a
`NestedAlgorithm`.

Algorithm selection follows `MatrixAlgebraKit.select_algorithm`. Generic
`default_algorithm(f; kwargs...)` and `select_algorithm(f, alg; kwargs...)`
helpers live in `AlgorithmsInterfaceExtensions`; operations register a
default by overloading `default_algorithm(::typeof(f); kwargs...)`. BP
overloads both for `message_update!`. Callers can pass either an explicit
`MessageUpdateAlgorithm` instance, a `NamedTuple` of keyword arguments
forwarded to the default algorithm, or `nothing` plus flat kwargs (also
forwarded to the default). A convenience signature
`message_update!(cache, factors, edge; alg = nothing, kwargs...)` routes
through `AIE.select_algorithm` so the same dispatch pattern is reachable
from a free-standing call. The `message_update_algorithm` kwarg on
`beliefpropagation` uses the same selector.

To keep the message-store iterate persistent across outer-loop iterations
without duplicating it on the outer state, introduce an `AIE.NestedState`
abstract type with generic `getproperty` / `setproperty!` /
`propertynames` forwarders for `:iterate` through a `:substate` field.
`BeliefPropagationState <: AIE.NestedState` now wraps a
`BeliefPropagationSweepState` as `substate`; the outer state holds no
`iterate` field of its own. The default `AIE.finalize_substate!` (which
copies `substate.iterate` back to `state.iterate`) becomes a self-write
through the forwarder — harmless and removes the need for a BP-specific
override.

Rename: `BeliefPropagationAlgorithm.sweep_algorithm` field →
`subalgorithm` (matches the generic name already used in
`AIE.initialize_subsolve`'s return tuple).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Refine the MAK-style algorithm selector introduced in the previous commit:

* `select_algorithm` / `default_algorithm` now take an `args` tuple as
  their final positional argument (after `alg`). A generic value→type
  wrapper forwards `(::Tuple)` to `(::Type{<:Tuple})`, so operations
  dispatch on the input-tuple type. Wrapping in a tuple keeps the value
  and type domains disjoint — `(1.2,)` is unambiguously a value tuple,
  `Tuple{Float64}` is unambiguously the type form. This matters at sites
  like `beliefpropagation` where not every input has a concrete value
  yet (no specific `edge`), and the type-form call simply passes
  `edgetype(factors)` in its slot.

* New `AIE.AbstractAlgorithm` supertype carries the
  generic "passthrough an explicit algorithm instance" overload of
  `select_algorithm`, so each operation doesn't have to repeat that
  method. `MessageUpdateAlgorithm <: AIE.AbstractAlgorithm`.

* BP call sites updated: `beliefpropagation` constructs the cache earlier
  and uses `Tuple{typeof(cache), typeof(factors), edgetype(factors)}` as
  the args type. The per-call `message_update!(cache, factors, edge; ...)`
  uses the value tuple `(cache, factors, edge)`. Both describe the same
  call shape.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.

2 participants