Derive frozen-rows transition mask lazily so it stays out of vars()/get_params()#43
Open
edeno wants to merge 1 commit into
Open
Derive frozen-rows transition mask lazily so it stays out of vars()/get_params()#43edeno wants to merge 1 commit into
edeno wants to merge 1 commit into
Conversation
_frozen_discrete_transition_rows_mask_ was stored as an instance attribute in __init__, so it leaked into vars() and get_params(). Serialization round-trips that rebuild a model from its public parameters (e.g. Spyglass's DecodingParameters, which stores vars(model) and reconstructs with Detector(**params)) then passed it back into __init__ as an unexpected keyword argument and raised TypeError. Compute the mask lazily via a read-only property instead (recomputed from frozen_discrete_transition_rows on access); keep the up-front validation in __init__ so bad input still raises at construction. Behavior is unchanged (existing constructor + slow EM frozen-row tests pass); the derived attribute no longer appears in vars()/get_params(), so ContFrag* defaults now reconstruct cleanly through the base detector. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
_DetectorBase.__init__stores_frozen_discrete_transition_rows_mask_-- aboolean row mask derived from the
frozen_discrete_transition_rowsconstructorargument -- as an instance attribute. Because it is a normal instance attribute,
it appears in both
vars(model)andget_params(), even though it is not aconstructor parameter.
This breaks serialization round-trips that reconstruct a model from its
attributes. For example, Spyglass's
DecodingParametersstoresvars(model)and later reconstructs the model with
Detector(**params); the derived mask ispassed back into
__init__and raises:Any consumer that treats a model's public attributes as its constructor
parameters hits the same problem, and it will recur whenever a new derived
attribute is added in
__init__.Solution
Compute the mask lazily via a read-only
_frozen_discrete_transition_rows_mask_property (recomputed from
frozen_discrete_transition_rowsand the state counton access) instead of storing it as an instance attribute in
__init__. Theup-front validation of
frozen_discrete_transition_rowsagainstn_statesiskept, so invalid input still raises at construction time. The derived value is
unchanged; it simply no longer appears in
vars()/get_params().Validation
unchanged (behavior preserved, including byte-exact frozen rows after EM).
vars()andget_params()for thedefault models, that the property still returns the correct mask (and
Nonewhen no rows are frozen), and that a ContFrag model's
vars()nowreconstructs through the base detector.
ruff check/ruff formatclean.Risks / possible issues
handful of times per fit/predict (discrete transition-matrix construction) and
_normalize_frozen_discrete_transition_rowsis inexpensive, so there is nomeaningful performance impact; it is not on a per-timestep hot path.
frozen_discrete_transition_rowsisstill rejected at construction.
vars()/get_params()-based consumers for the baseand ContFrag models. The
NonLocal*models additionally expose subclass-onlyparameters (
non_local_position_penalty,non_local_penalty_std); a pairedSpyglass change reconstructs via the concrete model class so those round-trip
as well. The two changes are independently beneficial.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com