Conversation
ia7ck
force-pushed
the
doubling
branch
3 times, most recently
from
b31af0d to
f7c578d
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new doubling library crate to the workspace, implementing a doubling table for functional-graph-style state transitions with aggregated per-transition values.
Changes:
- Introduces
Doubling<V>andTransition<V>types plus aValuetrait for composing transition values. - Adds
foldAPI to apply up tomax_stepstransitions from a starting state. - Adds unit tests and a proptest property test; registers crate with its own
Cargo.toml.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libs/doubling/src/lib.rs | Implements the doubling table, public API, docs, and tests. |
| libs/doubling/Cargo.toml | Defines the new doubling crate and its dev-dependency on proptest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Self { next, value } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Value::op is used to precompute aggregated values for 2^k steps and then combined in different parenthesizations depending on step. For correctness this operation must be associative (and conceptually a semigroup/monoid operation), but that requirement isn’t documented on the trait, so callers could implement a non-associative op and get incorrect results.
| /// A value that can be aggregated along transitions. | |
| /// | |
| /// `op` is used internally by `Doubling` to combine values for multiple steps. | |
| /// The combination is performed in different parenthesizations depending on | |
| /// the number of steps, so for correctness **`op` must be associative**: | |
| /// | |
| /// ```text | |
| /// (a.op(&b)).op(&c) == a.op(&b.op(&c)) | |
| /// ``` | |
| /// | |
| /// If this property does not hold, the result of operations like `fold` may | |
| /// depend on how the computation is grouped and therefore become incorrect. |
| /// 状態`start`から`step`回の遷移、初期値`init`から始めて`f`で畳みこんだ結果を返します。 | ||
| pub fn fold<A, F>(&self, start: usize, step: usize, init: A, f: F) -> A | ||
| where | ||
| F: Fn(A, &Transition<V>) -> A, | ||
| { | ||
| assert!(start < self.n_state); | ||
| assert!(step <= self.max_steps); | ||
|
|
||
| let mut i = start; | ||
| let mut acc = init; | ||
| for k in 0..=self.log2_max_steps { | ||
| if step >> k & 1 == 1 { | ||
| let offset = self.n_state * k; | ||
| let t = &self.transitions[offset + i]; | ||
| (i, acc) = (t.next, f(acc, t)); | ||
| } | ||
| } | ||
|
|
||
| acc | ||
| } |
There was a problem hiding this comment.
The current fold API/docstring reads like it folds over each of the step single-step transitions, but the implementation only calls f once per set bit (on aggregated 2^k transitions). This produces surprising/incorrect results for many plausible f implementations (e.g., counting steps). Either tighten the API to return the aggregated V (and final state) via Value::op, or document the required law for f (it must be compatible with Value::op / aggregated blocks) and consider renaming to avoid implying per-step folding.
| pub fn new<F>(n_state: usize, max_steps: usize, step1: F) -> Self | ||
| where | ||
| F: Fn(usize) -> Transition<V>, | ||
| { | ||
| let log2_max_steps = if max_steps == 0 { | ||
| 0 | ||
| } else { | ||
| max_steps.ilog2() as usize | ||
| }; | ||
|
|
||
| let mut transitions = Vec::with_capacity(n_state * (log2_max_steps + 1)); | ||
| for i in 0..n_state { | ||
| let t = step1(i); | ||
|
|
||
| assert!(t.next < n_state); | ||
|
|
||
| transitions.push(t); | ||
| } |
There was a problem hiding this comment.
When max_steps == 0, fold can only ever be called with step == 0, but new still eagerly calls step1 for every state and stores the level-0 transitions even though they can never be observed. Consider short-circuiting the construction for max_steps == 0 (e.g., store an empty table or skip calling step1) to avoid surprising side effects and unnecessary work.
1 participant
No description provided.