Skip to content

Simplify normalize_regime_transition_probs and other cleanups#214

Merged
hmgaudecker merged 12 commits intofix-136from
fix-187
Jan 22, 2026
Merged

Simplify normalize_regime_transition_probs and other cleanups#214
hmgaudecker merged 12 commits intofix-136from
fix-187

Conversation

@hmgaudecker
Copy link
Member

@hmgaudecker hmgaudecker commented Jan 18, 2026

Address some parts of #187. This simplifies the code a bit. I did not benchmark, but I do not see where it could lead to a slowdown.

@hmgaudecker hmgaudecker requested a review from timmens January 18, 2026 21:28
@hmgaudecker hmgaudecker linked an issue Jan 18, 2026 that may be closed by this pull request
@hmgaudecker hmgaudecker changed the title Regime transitions with array returns. Regime transitions with array returns, simplify regime passing. Jan 19, 2026
Copy link
Member

@timmens timmens left a comment

Choose a reason for hiding this comment

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

TBH, I am confused by the PR description / title and the code changes.

  1. We now have two normalize regime transition probs functions. The one that is tested is not used in the code base, and the one that is used is not tested.
  2. Given your statement that the regime passing is simplified, I would've expected to see changes in the models. This is actually done in #213 right?

@hmgaudecker hmgaudecker changed the title Regime transitions with array returns, simplify regime passing. Regime transitions with array returns Jan 22, 2026
Copy link
Member

@timmens timmens left a comment

Choose a reason for hiding this comment

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

Issue #187 asks to:

Use the Array-return interface also internally [for regime transitions]

The issue notes that after #186, the user-facing interface supports returning Arrays directly, but internally the code still relies on dict[str, float | Array] format.

What the PR actually does:

  1. Changes type annotations from dict to MappingProxyType and list to tuple
  2. Refactors normalize_regime_transition_probs to work with MappingProxyType[str, Array] instead of dict[str, float | Array]
  3. Various cleanup and refactoring

What's still dict-based internally:

  1. In src/lcm/simulation/util.py:160-168:
    regime_transition_probs: MappingProxyType[str, Array] = (
    internal_regime.internal_functions.regime_transition_probs.simulate(...)
    )
  2. In src/lcm/Q_and_F.py:138-142:
    regime_transition_prob: MappingProxyType[str, Array] = (
    regime_transition_prob_func(...)
    )
  3. normalize_regime_transition_probs still takes and returns MappingProxyType[str, Array]
  4. draw_key_from_dict still consumes a dict of arrays

The core internal representation remains dict[RegimeName, Array] (now wrapped in MappingProxyType). To actually fix issue 187, the internal
interface would need to change to use a single Array with shape (n_points, n_regimes) instead of a dict mapping regime names to probability
arrays.


While this is something we could do, I actually don't think it has a high priority right now.

@hmgaudecker hmgaudecker changed the title Regime transitions with array returns Simplify normalize_regime_transition_probs and other cleanups Jan 22, 2026
@hmgaudecker hmgaudecker merged commit ae67a4d into fix-136 Jan 22, 2026
8 checks passed
@hmgaudecker hmgaudecker deleted the fix-187 branch January 22, 2026 16:28
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