Simplify normalize_regime_transition_probs and other cleanups#214
Simplify normalize_regime_transition_probs and other cleanups#214hmgaudecker merged 12 commits intofix-136from
normalize_regime_transition_probs and other cleanups#214Conversation
…ch clearer what is meant than when we do object.
timmens
left a comment
There was a problem hiding this comment.
TBH, I am confused by the PR description / title and the code changes.
- 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.
- 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?
timmens
left a comment
There was a problem hiding this comment.
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:
- Changes type annotations from dict to MappingProxyType and list to tuple
- Refactors normalize_regime_transition_probs to work with MappingProxyType[str, Array] instead of dict[str, float | Array]
- Various cleanup and refactoring
What's still dict-based internally:
- In src/lcm/simulation/util.py:160-168:
regime_transition_probs: MappingProxyType[str, Array] = (
internal_regime.internal_functions.regime_transition_probs.simulate(...)
) - In src/lcm/Q_and_F.py:138-142:
regime_transition_prob: MappingProxyType[str, Array] = (
regime_transition_prob_func(...)
) - normalize_regime_transition_probs still takes and returns MappingProxyType[str, Array]
- 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.
normalize_regime_transition_probs and other cleanups
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.