-
Notifications
You must be signed in to change notification settings - Fork 18
Add RLSSMConfig class for reinforcement learning models #861
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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 the RLSSMConfig class, a specialized configuration class for Reinforcement Learning Sequential Sampling Models (RLSSM) that inherits from the BaseModelConfig abstract base class. The implementation provides a flexible and well-tested configuration system tailored to the unique needs of RLSSM models.
Key changes:
- Adds
RLSSMConfigclass with RLSSM-specific fields likeparams_default(list format),decision_process,learning_process, and validation counts - Implements factory method
from_rlssm_dict()to create configs from existing RLSSM dictionary structures - Provides
to_config()method for backward compatibility with standardConfigclass - Includes comprehensive test suite with 25 tests achieving 100% coverage
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/hssm/config.py | Adds the RLSSMConfig class (lines 244-460) with validation, conversion, and property methods for RLSSM model configuration |
| tests/test_rlssm_config.py | Comprehensive test suite covering creation, validation, defaults retrieval, conversion to Config, and data alias functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… Config validation
AlexanderFengler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is on the right track, but it's not yet being tested with integration right? Left a few comments.
There was a problem hiding this 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 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…fig instantiation
AlexanderFengler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few very small comments. Thanks @cpaniaguam
src/hssm/config.py
Outdated
| model_name=model_name, | ||
| loglik_kind=loglik_kind, | ||
| response=["rt", "response"], | ||
| response=DEFAULT_RLSSM_RESPONSE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per comment above, I think it's semantically a bit confusing to have the RLSSM default show up here.
Also sidenote that I just notice now.
We are doing a redefinition of reponse --> ['rt', 'response'] here, which is a bit confusing.
Maybe renaming this argument to observed_data_vars makes more sense globally. (This can be either ["response"] or ["rts", "response"] or even something entirely different later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @AlexanderFengler. I think @krishnbera was using the name data for this. However, in HSSM model configs response was being used already. In the interest of being more consistent I sort of defaulted to
response = ["rt", "response"]for this config class. We can use observed_data_vars as you suggest. Again, we can make changes to this later as we actually start to use it to build models. Thoughts?
krishnbera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
This PR introduces the
RLSSMConfigclass, a specialized configuration class for Reinforcement Learning Sequential Sampling Models (RLSSM) that inherits from the newly createdBaseModelConfigabstract base class (#860).Key Features
RLSSMConfig Class
BaseModelConfig: Leverages the common configuration interface while adding RLSSM-specific functionalityparams_default: List of default parameter values (matcheslist_paramsorder)decision_process: Reference to the decision model (e.g., "LAN")learning_process: Dictionary of learning functions for computed parametersn_params,n_extra_fields: Validation counts for parameter consistencydecision_model,lan_model: Additional metadata for RLSSM modelsMethods
from_rlssm_dict(): Factory method to create configs from existing RLSSM dictionary structuresvalidate(): Comprehensive validation ensuring parameter counts, bounds, and required fields are consistentget_defaults(): Returns default values and bounds for parameters (returnsfloatinstead ofParamSpec)to_config(): ConvertsRLSSMConfigto standardConfigfor backward compatibilitydataproperty: Alias forresponsefield to support both naming conventionsKey Differences from Standard Config
params_default(list) instead ofdefault_priors(dict) to match RLSSM parameter orderingTesting
RLSSMConfigclassUsage Example
Related