Skip to content

Cpaniaguam/rlssm simplified interface#955

Open
cpaniaguam wants to merge 42 commits into
mainfrom
cpaniaguam/rlssm-simplified-interface
Open

Cpaniaguam/rlssm simplified interface#955
cpaniaguam wants to merge 42 commits into
mainfrom
cpaniaguam/rlssm-simplified-interface

Conversation

@cpaniaguam
Copy link
Copy Markdown
Collaborator

@cpaniaguam cpaniaguam commented May 6, 2026

flowchart TD
    subgraph SSM_Registry
      A1["_SSM_REGISTRY (dict)"]
      A2["_SSM_LOGP_CACHE (dict)"]
    end
    subgraph RLSSM_Registry
      B1["_RLSSM_REGISTRY (dict)"]
    end
    subgraph User_API
      C1[register_ssm]
      C2[register_rlssm_model]
      C3[get_rlssm_model_config]
    end
    C1-->|adds entry|A1
    C1-->|adds entry|A2
    C2-->|adds entry|B1
    C3-->|reads entry|B1
    C3-->|reads entry|A1
    C3-->|calls _get_ssm_logp|A2
    A2-->|lazy build if needed|A1
    C3-->|returns RLSSMConfig|D1["RLSSMConfig"]
    style D1 fill:#1e7a1e,stroke:#fff,stroke-width:2px,color:#fff
    style A1 fill:#1e3a7a,stroke:#fff,stroke-width:2px,color:#fff
    style A2 fill:#1e3a7a,stroke:#fff,stroke-width:2px,color:#fff
    style B1 fill:#7a1e1e,stroke:#fff,stroke-width:2px,color:#fff
    style C1 fill:#7a7a1e,stroke:#fff,stroke-width:2px,color:#fff
    style C2 fill:#7a7a1e,stroke:#fff,stroke-width:2px,color:#fff
    style C3 fill:#7a7a1e,stroke:#fff,stroke-width:2px,color:#fff
Loading

@cpaniaguam cpaniaguam marked this pull request as draft May 6, 2026 18:52
@cpaniaguam cpaniaguam requested a review from Copilot May 6, 2026 18:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a simplified public interface for RLSSM by adding a named-model registry and wrapping the existing RLSSM implementation behind a friendlier constructor that can build an RLSSMConfig from a model name (defaulting to "rldm").

Changes:

  • Split the existing RLSSM implementation into an internal base class (_RLSSM) and a public wrapper (RLSSM) with a simplified constructor.
  • Added an RLSSM/SSM registry and factory (get_rlssm_model_config, register_rlssm_model, register_ssm) to construct configs from named models.
  • Expanded the RLSSM test suite to cover the simplified interface, registry behavior, and the new wrapper semantics.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/test_rlssm.py Adds coverage for the new simplified RLSSM constructor and registry-based config creation.
src/hssm/rl/rlssm.py Renames the prior implementation to _RLSSM and adds the public RLSSM wrapper + blocked-attribute behavior.
src/hssm/rl/registry.py New registry/factory module for named RLSSM models and SSM base logp functions.
src/hssm/rl/__init__.py Exposes _RLSSM and registry helpers in the hssm.rl public API.
src/hssm/__init__.py Exposes register_rlssm_model at the top-level hssm API.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/hssm/rl/registry.py Outdated
Comment thread src/hssm/rl/rlssm.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comment thread src/hssm/rl/registry.py
Comment thread src/hssm/rl/registry.py
Comment thread src/hssm/rl/registry.py Outdated
cpaniaguam added 3 commits May 6, 2026 15:54
- Added KeyError for missing SSM in the registry.
- Improved ValueError messages for non-callable logp functions.
- Implemented runtime checks for list_params and loglik in RLSSM.
- Ensured defensive copying of mutable parameters in registry functions.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment thread src/hssm/rl/registry.py Outdated
Comment thread src/hssm/rl/registry.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comment thread tests/rl/test_rlssm.py Outdated
Comment thread tests/rl/test_rlssm.py Outdated
Comment thread tests/rl/test_rlssm.py Outdated
Comment thread tests/rl/test_rlssm.py Outdated
Comment thread tests/rl/test_rlssm.py Outdated
Copy link
Copy Markdown
Member

@AlexanderFengler AlexanderFengler left a comment

Choose a reason for hiding this comment

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

Left a few comments, mostly looks good already.
@krishnbera can you take a look too?

Comment thread src/hssm/rl/__init__.py
- ``RLSSMConfig``: the config class for RL + SSM models in :mod:`hssm.rl.config`.
- ``get_rlssm_model_config``: factory that builds a config from a named model.
- ``register_rlssm_model``: register a custom named RLSSM model.
- ``register_ssm``: register a custom SSM base logp function.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would we want/need a register_learning_process equivalent here? ( @krishnbera )

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes we should have a way of registering the learning process separately.

Comment thread src/hssm/rl/registry.py Outdated
Comment thread src/hssm/rl/registry.py
@cpaniaguam
Copy link
Copy Markdown
Collaborator Author

flowchart TD
    USER["User: RLSSM(model='rldm')"]

    RLSSM_REG["_RLSSM_REGISTRY\nnamed models\ne.g. 'rldm'"]

    SSM_REG["_SSM_REGISTRY\ncustom SSMs only\n(empty by default)"]

    MODELCONFIG["hssm.modelconfig\nbuilt-in SSMs\nddm · angle · weibull · ornstein · …"]

    CACHE["_SSM_LOGP_CACHE\nlazy ONNX → JAX fn"]

    OUTPUT["RLSSMConfig"]

    USER --> RLSSM_REG
    RLSSM_REG -- decision_process name --> LOOKUP

    LOOKUP{"registered\ncustom SSM?"}
    LOOKUP -- yes --> SSM_REG
    LOOKUP -- no  --> MODELCONFIG

    SSM_REG & MODELCONFIG --> CACHE
    CACHE -- annotated JAX fn --> OUTPUT
    RLSSM_REG -- rl params / bounds / learning process --> OUTPUT
Loading

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 11 changed files in this pull request and generated no new comments.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 11 changed files in this pull request and generated no new comments.

@cpaniaguam cpaniaguam marked this pull request as ready for review May 15, 2026 18:03
krishnbera
krishnbera previously approved these changes May 21, 2026
Copy link
Copy Markdown
Member

@krishnbera krishnbera left a comment

Choose a reason for hiding this comment

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

looks good overall. added minor comments.

Comment thread src/hssm/rl/registry.py
Comment thread src/hssm/rl/registry.py Outdated
Comment thread src/hssm/rl/registry.py Outdated
Comment thread src/hssm/rl/registry.py Outdated
Comment thread src/hssm/rl/registry.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 12 changed files in this pull request and generated 1 comment.

Comment thread src/hssm/rl/registry.py
Examples
--------
>>> import hssm
>>> hssm.rl.list_models()
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.

4 participants