lnccbrown / lnccbrown/ssm-simulators

Custom boundary without `boundary_params` fails at `simulate()` with `KeyError: "Boundary 'my_boundary' is not registered"`

Open
#309 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Jupyter Notebook
Stars
24
Forks
18
Avg merge
1d 14h
Merged PRs (30d)
9

Description

`Simulator("ddm", boundary=my_callable)` with no `boundary_params` constructs without warning, because `ddm` ships `"boundary_params": []` and the guard checks key presence rather than emptiness. `make_boundary_dict` then tests `boundary_params` for truthiness, takes the registry branch, and looks the callable's `__name__` up in the boundary registry. The user is told their function is not registered and shown a list of six built-in boundary names, which points at the wrong fix — the actual fix is the one-line `boundary_params=[...]`. `ModelConfigBuilder.add_boundary` already rejects the same mistake at the point it is made.

Environment: ssm-simulators 0.13.2, Python 3.12, macOS (Darwin 25.4.0).

## Reproducer

```python
import numpy as np

from ssms.basic_simulators.simulator_class import Simulator

def my_boundary(t, theta=0.2, scale=1.0):
return scale * np.maximum(1.0 - theta * t, 0.1)

# "ddm" ships "boundary_params": [], so the missing-params warning never fires.
sim = Simulator("ddm", boundary=my_boundary)
print("boundary_name :", sim.config["boundary_name"])
print("boundary_params:", sim.config["boundary_params"])

sim.simulate(theta={"v": 0.5, "a": 1.0, "z": 0.5, "t": 0.3}, n_samples=10)
```

## Output

```
boundary_name : my_boundary
boundary_params: []
Traceback (most recent call last):
File "/…/snippet2.py", line 15, in
sim.simulate(theta={"v": 0.5, "a": 1.0, "z": 0.5, "t": 0.3}, n_samples=10)
File "/…/site-packages/ssms/basic_simulators/simulator_class.py", line 651, in simulate
boundary_dict = make_boundary_dict(model_config_local, theta)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/…/site-packages/ssms/basic_simulators/simulator.py", line 328, in make_boundary_dict
boundary_info = boundary_registry.get(boundary_name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/…/site-packages/ssms/config/boundary_registry.py", line 117, in get
raise KeyError(
KeyError: "Boundary 'my_boundary' is not registered. Available boundaries: ['addm_collapse', 'angle', 'conflict_gamma', 'constant', 'generalized_logistic', 'weibull_cdf']"
```

Related cases, from a second run that catches instead of crashing (verbatim excerpts):

```
shipped config for ddm boundary_params=[]
shipped config for addm boundary_params=[]
shipped config for angle boundary_params=''

=== B: Simulator('angle', boundary=), no boundary_params ===
warnings at construction: ["Custom boundary function provided without 'boundary_params'. You may need to specify bound"]
simulate RAISED KeyError: "Boundary 'my_boundary' is not registered. …"

=== C: documented form, boundary_params=['theta','scale'] ===
warnings at construction: []
OK -> rts[:3]: [0.548798 1.370101 1.313849]

=== D: parameter-free boundary with explicit boundary_params=[] ===
config['boundary_params']: []
simulate RAISED KeyError: "Boundary 'flat_boundary' is not registered. …"

=== E: ModelConfigBuilder.add_boundary(cfg, ) with no params ===
RAISED ValueError: Must provide boundary_params when using custom boundary function
```

Case D is the sharper edge: there is currently no value of `boundary_params` that expresses "my boundary takes no extra parameters" — `[]` fails identically to omitting it — so that shape of custom boundary is unreachable through `Simulator`.

## Why

- `ssms/basic_simulators/simulator_class.py:388-400` — callable branch of `_apply_custom_boundary`: sets `config["boundary_name"] = fn.__name__` (392), and warns + sets `config["boundary_params"] = []` only `if "boundary_params" not in config` (394-400). `ddm` and `addm` ship the key with value `[]`, so no warning.
- `ssms/basic_simulators/simulator_class.py:206-216` — `_build_config` applies `config.update(config_overrides)` before `_apply_custom_boundary`, so a user-supplied `boundary_params` and a base model's shipped one are indistinguishable at the point of the check.
- `ssms/basic_simulators/simulator.py:303` — `if callable(config.get("boundary")) and config.get("boundary_params"):` — truthiness, so `[]` falls through to the registry branch.
- `ssms/basic_simulators/simulator.py:326-328` → `ssms/config/boundary_registry.py:115-120` — registry lookup on the function's `__name__`, raising the misleading `KeyError`.
- Correct behaviour already exists at `ssms/config/model_config_builder.py:405-412`.

## Suggested fix

Two independent changes. First, dispatch on presence rather than truthiness so a callable boundary never falls through to a registry lookup on a function name, which also makes `boundary_params=[]` mean "no extra parameters":

```diff
- if callable(config.get("boundary")) and config.get("boundary_params"):
+ if callable(config.get("boundary")) and config.get("boundary_params") is not None:
```

Second, fail fast at construction the way `add_boundary` does, checking the user's own `config_overrides` rather than the merged config so a base model's shipped `[]` cannot suppress it:

```python
elif callable(boundary):
self._validate_boundary_function(boundary)
if "boundary_params" not in config_overrides:
raise ValueError(
f"Custom boundary {getattr(boundary, '__name__', 'custom')!r} requires an "
"explicit boundary_params list, e.g. Simulator('ddm', boundary=my_boundary, "
"boundary_params=['theta', 'scale']). Pass [] if it takes no extra parameters."
)
config["boundary_params"] = config_overrides["boundary_params"]
```

Optionally, in the else branch at `simulator.py:326-328`, if `config["boundary"]` is callable but the registry lookup fails, re-raise naming the real problem instead of the registry's "not registered" text.

## Is this intended?

The documented form works cleanly — case C above constructs with no warnings and simulates — so this is not a docs bug, and `config["boundary_name"] = fn.__name__` is defensible on its own as a readable metadata label. There is also already a warning on models whose config omits `boundary_params` (case B), which a maintainer may consider sufficient. The counter is that `ddm` and `addm` get no warning at all, and that even when the warning does fire it is emitted at construction while the failure arrives later at `simulate()`, misattributed to the registry.

Two caveats. The truthiness guard at `simulator.py:303` looks deliberate — the comment at 304-307 says it exists so process-local custom boundaries survive multiprocessing workers that start with a fresh registry; `is not None` preserves that intent, whereas dropping the second clause entirely would not. And the construction-time raise is a behaviour change for code that builds a `Simulator` with a callable boundary and only introspects `.config` without simulating; that combination is already non-functional, but it is technically a break. I exercised only the `Simulator` path — I did not survey whether `ssms.dataset_generators` or `hssm_support` reach `make_boundary_dict` with a callable boundary and empty `boundary_params`.
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with _apply_custom_boundary and _build_config in ssms/basic_simulators/simulator_class.py, then trace make_boundary_dict in ssms/basic_simulators/simulator.py and the validation in ssms/config/model_config_builder.py. Run the provided Simulator reproducer for omitted, empty, and explicit boundary_params; done means custom boundaries fail at construction when parameters are unspecified and valid parameter-free boundaries do not fall into registry lookup.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.