lnccbrown / lnccbrown/ssm-simulators

`add_boundary` / `add_drift` / `Simulator` store the registry's own `params` list in the returned config, so editing it corrupts the global registry

Open
#311 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

`BoundaryRegistry.get` and `DriftRegistry.get` return the stored spec dict without copying, and four call sites assign `spec["params"]` straight into a user-facing config. An in-place edit such as `cfg["boundary_params"].append("t")` therefore mutates the process-wide registry. Every later config built from that boundary name inherits the change, and because `make_boundary_dict` reads the registry (not the config) at simulate time, untouched built-in models start failing.

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

## Reproducer

```python
import ssms
from ssms.config import ModelConfigBuilder, get_boundary_registry, get_drift_registry
from ssms.basic_simulators.simulator import simulator
from ssms.basic_simulators.simulator_class import Simulator

print("ssms:", ssms.__version__)
breg, dreg = get_boundary_registry(), get_drift_registry()
print("registry angle params (pristine):", breg.get("angle")["params"])

# --- 1. add_boundary aliases the registry's list into the user's config -------
cfg = ModelConfigBuilder.add_boundary(ModelConfigBuilder.from_model("ddm"), "angle")
print("cfg['boundary_params'] IS registry list:",
cfg["boundary_params"] is breg.get("angle")["params"])

cfg["boundary_params"].append("t") # edit *my* config, not the registry
print("registry angle params after my edit:", breg.get("angle")["params"])

# --- 2. the corruption is global and permanent for the process ---------------
print("a fresh, unrelated config:",
ModelConfigBuilder.add_boundary(
ModelConfigBuilder.from_model("ddm"), "angle")["boundary_params"])
try:
simulator(model="angle", theta=[1.0, 1.0, 0.5, 0.3, 0.2], n_samples=10)
except Exception as e:
print("simulator(model='angle') ->", type(e).__name__ + ":", e)

# --- 3. same aliasing for drift ---------------------------------------------
d = ModelConfigBuilder.add_drift(ModelConfigBuilder.from_model("ddm"), "gamma_drift")
print("cfg['drift_params'] IS registry list:",
d["drift_params"] is dreg.get("gamma_drift")["params"])

# --- 4. same aliasing via Simulator (models with no boundary_params key) -----
s = Simulator(model="ddm_sdv", boundary="weibull_cdf")
print("Simulator._config['boundary_params'] IS registry list:",
s._config["boundary_params"] is breg.get("weibull_cdf")["params"])

# --- 5. from_model does NOT alias: registry.get() deep-copies ----------------
a, b = ModelConfigBuilder.from_model("ddm"), ModelConfigBuilder.from_model("ddm")
print("from_model aliasing:",
{k: (a[k] is b[k]) for k in ("params", "param_bounds", "default_params")})

# --- 6. stale docstring ------------------------------------------------------
import inspect
print("'multiplicative' in docstring:",
"multiplicative" in inspect.getdoc(ModelConfigBuilder.add_boundary),
"| in signature:",
"multiplicative" in inspect.signature(ModelConfigBuilder.add_boundary).parameters)
```

## Output

```
ssms: 0.13.2
registry angle params (pristine): ['a', 'theta']
cfg['boundary_params'] IS registry list: True
registry angle params after my edit: ['a', 'theta', 't']
a fresh, unrelated config: ['a', 'theta', 't']
simulator(model='angle') -> TypeError: function() got multiple values for keyword argument 't'
cfg['drift_params'] IS registry list: True
Simulator._config['boundary_params'] IS registry list: True
from_model aliasing: {'params': False, 'param_bounds': False, 'default_params': False}
'multiplicative' in docstring: True | in signature: False
```

In a clean process the same `simulator(model="angle", ...)` call succeeds, so the `TypeError` is caused by the leak.

## Why

Getters hand out the stored object:

- `ssms/config/boundary_registry.py:121` — `return self._boundaries[name]`
- `ssms/config/drift_registry.py:117` — `return self._drifts[name]`
- `register()` at `boundary_registry.py:81-84` / `drift_registry.py:77-80` also stores the caller's `params` list by reference.

Four sites put that list into a user-facing config:

- `ssms/config/model_config_builder.py:404` — `config["boundary_params"] = boundary_spec["params"]`
- `ssms/config/model_config_builder.py:458` — `config["drift_params"] = drift_spec["params"]`
- `ssms/basic_simulators/simulator_class.py:387` — `config["boundary_params"] = boundary_info["params"]`
- `ssms/basic_simulators/simulator_class.py:431` — `config["drift_params"] = drift_info["params"]`

The consumer that then reads the corrupted registry is `make_boundary_dict` (`ssms/basic_simulators/simulator.py:326-332`), which filters `theta` by `boundary_info["params"]` at simulate time.

Blast radius of the `Simulator` site is wide: 111 of 113 built-in model configs lack a `boundary_params` key, so the `if "boundary_params" not in config` guard at `simulator_class.py:387` fires for nearly every model.

Stale docstring, separately: `model_config_builder.py:376-377` documents a `multiplicative` parameter that does not exist in the signature or anywhere in the package. The same claim appears in comments at `ssms/external_simulators/pyddm_mapper.py:201,222`.

## Suggested fix

Copy at the root so all four call sites are covered at once:

```python
# ssms/config/boundary_registry.py, BoundaryRegistry.get
spec = self._boundaries[name]
return {"fun": spec["fun"], "params": list(spec["params"])}

# ssms/config/drift_registry.py, DriftRegistry.get -- same shape
```

Copy on the way in too (`self._boundaries[name] = {"fun": function, "params": list(params)}`), and add belt-and-braces `list(...)` at the four assignment sites, since those are what land in long-lived user-facing dicts. Delete `model_config_builder.py:376-377` and the two matching `pyddm_mapper.py` comments. Regression test:

```python
def test_add_boundary_does_not_alias_registry():
reg = get_boundary_registry()
before = list(reg.get("angle")["params"])
cfg = ModelConfigBuilder.add_boundary(ModelConfigBuilder.from_model("ddm"), "angle")
cfg["boundary_params"].append("x")
assert reg.get("angle")["params"] == before
```

## Is this intended?

If the intended contract is "configs are opaque, replace don't edit", this is arguably working as designed — a user who writes `cfg["boundary_params"] = [...]` is unaffected. Two things argue against that reading: nothing documents it, and `add_boundary`'s own docstring frames the config as modified in place; and the package has already settled on copy-on-read for config lookup elsewhere — `ModelConfigRegistry.get` deep-copies (`model_registry.py:160`) and `ssms.config.model_config` is a `CopyOnAccessDict` (`config/__init__.py:53-60`). The boundary and drift registries are the two that do not honour it.

Two scope notes so the report is not overstated. `from_model` is **not** affected (verified above); do not read this as a general config-aliasing bug. And the exact `TypeError` depends on appending a name that collides with a real simulator kwarg (`"t"`) — an arbitrary name corrupts the registry just as thoroughly but may surface later or differently, so the `is` identity check is the stable evidence. A subtractive edit would silently drop a parameter from the boundary call rather than raise.

Adjacent, probably a separate issue: `Simulator(model="ddm", boundary="angle")` leaves `_config["boundary_params"] == []`, because `ddm` already has that key so the guard at `simulator_class.py:387` skips — the boundary function is swapped but its parameter list is not. The `list(...)` fix does not address that.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the registry getters and registration paths in ssms/config/boundary_registry.py and drift_registry.py, then trace the four assignments in model_config_builder.py and basic_simulators/simulator_class.py. Run the reported regression scenario and add coverage for independent config and registry parameter lists; verify simulator behavior remains valid and review the stale docstring references in model_config_builder.py and pyddm_mapper.py.

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
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.