refactor: simplify around connector-first SIMIND adaptors#1
Merged
samdporter merged 14 commits intomainfrom Mar 5, 2026
Merged
refactor: simplify around connector-first SIMIND adaptors#1samdporter merged 14 commits intomainfrom
samdporter merged 14 commits intomainfrom
Conversation
Reviewer's GuideRefactors the SIMIND integration to a connector-first architecture by routing STIR/SIRF/PyTomography adaptors through SimindPythonConnector, inlining voxel-phantom and energy-window setup into the connector core, removing the legacy SimindSimulator/native backend layer, and updating examples, tests, Dockerfiles, configs, and docs to match the new adaptor-focused API. Sequence diagram for STIR adaptor run via SimindPythonConnectorsequenceDiagram
actor User
participant StirSimindAdaptor
participant SimindPythonConnector
participant SimulationConfig
participant RuntimeSwitches
participant SimindExecutor
participant SIMIND
participant STIR
User->>StirSimindAdaptor: __init__(config_source, output_dir, ...)
StirSimindAdaptor->>SimindPythonConnector: __init__(config_source, output_dir, ...)
SimindPythonConnector->>SimulationConfig: load or clone config
SimindPythonConnector->>RuntimeSwitches: initialize
User->>StirSimindAdaptor: set_source(stir_image)
StirSimindAdaptor-->>StirSimindAdaptor: store _source
User->>StirSimindAdaptor: set_mu_map(stir_mu_map)
StirSimindAdaptor-->>StirSimindAdaptor: store _mu_map
User->>StirSimindAdaptor: set_energy_windows(lowers, uppers, orders)
StirSimindAdaptor->>SimindPythonConnector: set_energy_windows(lowers, uppers, orders)
User->>StirSimindAdaptor: add_runtime_switch("NN", photon_multiplier)
StirSimindAdaptor->>SimindPythonConnector: add_runtime_switch("NN", photon_multiplier)
SimindPythonConnector->>RuntimeSwitches: set_switch("NN", photon_multiplier)
User->>StirSimindAdaptor: run(runtime_operator)
StirSimindAdaptor-->>StirSimindAdaptor: _validate_inputs()
StirSimindAdaptor-->>StirSimindAdaptor: _extract_voxel_size_mm(_source)
StirSimindAdaptor-->>StirSimindAdaptor: convert source, mu_map to numpy
StirSimindAdaptor->>SimindPythonConnector: configure_voxel_phantom(source_array, mu_array, voxel_size_mm, scoring_routine)
SimindPythonConnector->>SimulationConfig: set_flag, set_value, set_data_file
SimindPythonConnector->>RuntimeSwitches: set_switch("PX", vox_cm)
SimindPythonConnector-->>StirSimindAdaptor: source_path, density_path
StirSimindAdaptor->>SimindPythonConnector: run(runtime_operator)
SimindPythonConnector->>SimindExecutor: run_simulation(output_prefix, orbit_file, runtime_switches)
SimindExecutor->>SIMIND: invoke subprocess simind output_prefix ...
SIMIND-->>SimindExecutor: projection files
SimindExecutor-->>SimindPythonConnector: return
SimindPythonConnector-->>SimindPythonConnector: load_interfile_array(...)
SimindPythonConnector-->>StirSimindAdaptor: raw_outputs Dict
StirSimindAdaptor-->>StirSimindAdaptor: wrap headers as stir.ProjData
StirSimindAdaptor-->>User: outputs Dict~str, ProjData~
Class diagram for updated connector and adaptor typesclassDiagram
class BaseConnector {
<<abstract>>
+add_config_value(index int, value Any) void
+add_runtime_switch(switch str, value Any) void
+set_runtime_switches(switches Mapping) void
+run(*args Any, **kwargs Any) Any
+get_outputs() Dict~str, Any~
+get_config() SimulationConfig
}
class SimulationConfig {
+set_value(index int, value Any) void
+set_flag(index int, value bool) void
+set_data_file(index int, prefix str) void
+get_value(key Any) Any
+get_flag(index int) bool
}
class RuntimeSwitches {
+set_switch(key str, value Any) void
+get_all() Dict~str, Any~
}
class SimindExecutor {
+logger
+__init__() void
+run_simulation(output_prefix str, orbit_file Path, runtime_switches Dict) void
}
class SimindPythonConnector {
+config SimulationConfig
+output_dir Path
+output_prefix str
+quantization_scale float
+runtime_switches RuntimeSwitches
+executor SimindExecutor
+add_config_value(index int, value Any) void
+add_runtime_switch(switch str, value Any) void
+configure_voxel_phantom(source ndarray, mu_map ndarray, voxel_size_mm float, scoring_routine ScoringRoutine) tuple~Path, Path~
+set_energy_windows(lower_bounds Any, upper_bounds Any, scatter_orders Any) void
+run(runtime_operator RuntimeOperator) Dict~str, ProjectionResult~
+get_config() SimulationConfig
}
class StirSimindAdaptor {
+python_connector SimindPythonConnector
+_scoring_routine ScoringRoutine
+_source Any
+_mu_map Any
+_outputs Dict~str, Any~
+__init__(config_source ConfigSource, output_dir str, output_prefix str, photon_multiplier int, quantization_scale float, scoring_routine ScoringRoutine) void
+set_source(source Any) void
+set_mu_map(mu_map Any) void
+set_energy_windows(lower_bounds Any, upper_bounds Any, scatter_orders Any) void
+add_config_value(index int, value Any) void
+add_runtime_switch(switch str, value Any) void
+run(runtime_operator RuntimeOperator) Dict~str, Any~
+get_outputs() Dict~str, Any~
+get_total_output(window int) Any
+get_scatter_output(window int) Any
+get_primary_output(window int) Any
+get_air_output(window int) Any
+get_penetrate_output(component PenetrateOutputType) Any
+list_available_outputs() list~str~
+get_scoring_routine() ScoringRoutine
+get_config() SimulationConfig
-_get_component(prefix str, window int) Any
-_extract_voxel_size_mm(image Any) float
-_validate_inputs() void
}
class SirfSimindAdaptor {
+python_connector SimindPythonConnector
+_scoring_routine ScoringRoutine
+_source Any
+_mu_map Any
+_outputs Dict~str, Any~
+__init__(config_source ConfigSource, output_dir str, output_prefix str, photon_multiplier int, quantization_scale float, scoring_routine ScoringRoutine) void
+set_source(source Any) void
+set_mu_map(mu_map Any) void
+set_energy_windows(lower_bounds Any, upper_bounds Any, scatter_orders Any) void
+add_config_value(index int, value Any) void
+add_runtime_switch(switch str, value Any) void
+run(runtime_operator RuntimeOperator) Dict~str, Any~
+get_outputs() Dict~str, Any~
+get_total_output(window int) Any
+get_scatter_output(window int) Any
+get_primary_output(window int) Any
+get_air_output(window int) Any
+get_penetrate_output(component PenetrateOutputType) Any
+list_available_outputs() list~str~
+get_scoring_routine() ScoringRoutine
+get_config() SimulationConfig
-_get_component(prefix str, window int) Any
-_extract_voxel_size_mm(image Any) float
-_validate_inputs() void
}
class PyTomographySimindAdaptor {
+python_connector SimindPythonConnector
+output_dir Path
+output_prefix str
+voxel_size_mm float
+_scoring_routine ScoringRoutine
+_source torch.Tensor
+_mu_map torch.Tensor
+_energy_windows tuple
+_outputs Dict~str, torch.Tensor~
+_output_header_paths Dict~str, Path~
+__init__(config_source ConfigSource, output_dir PathLike, output_prefix str, voxel_size_mm float, photon_multiplier int, quantization_scale float, scoring_routine ScoringRoutine) void
+set_source(source torch.Tensor) void
+set_mu_map(mu_map torch.Tensor) void
+set_energy_windows(lower_bounds list~float~, upper_bounds list~float~, scatter_orders list~int~) void
+add_config_value(index int, value Any) void
+add_runtime_switch(switch str, value Any) void
+run(runtime_operator RuntimeOperator) Dict~str, torch.Tensor~
+get_outputs() Dict~str, torch.Tensor~
+get_output_header_path(key str) Path
+get_total_output(window int) torch.Tensor
+get_scatter_output(window int) torch.Tensor
+get_primary_output(window int) torch.Tensor
+get_air_output(window int) torch.Tensor
+list_available_outputs() list~str~
-_get_component(prefix str, window int) torch.Tensor
-_validate_inputs() void
-_validate_tensor(value torch.Tensor, name str) torch.Tensor
+from_simind_image_axes(value torch.Tensor) torch.Tensor
+to_simind_image_axes(value torch.Tensor) torch.Tensor
}
BaseConnector <|.. SimindPythonConnector
BaseConnector <|-- StirSimindAdaptor
BaseConnector <|-- SirfSimindAdaptor
BaseConnector <|-- PyTomographySimindAdaptor
SimindPythonConnector o-- SimulationConfig
SimindPythonConnector o-- RuntimeSwitches
SimindPythonConnector o-- SimindExecutor
StirSimindAdaptor o-- SimindPythonConnector
SirfSimindAdaptor o-- SimindPythonConnector
PyTomographySimindAdaptor o-- SimindPythonConnector
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 4 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The STIR and SIRF adaptors now have near-identical logic (source/mu handling, energy window plumbing, output accessors, voxel-size extraction); consider factoring this into a shared helper/base class to reduce duplication and keep future behavior changes consistent across backends.
- The STIR adaptor’s
_extract_voxel_size_mmfalls back toget_grid_spacing()[3]and the SIRF adaptor only supportsvoxel_sizes()[2]; it might be worth aligning these conventions and documenting/validating expected units/axis ordering to avoid subtle geometry mismatches between backends.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The STIR and SIRF adaptors now have near-identical logic (source/mu handling, energy window plumbing, output accessors, voxel-size extraction); consider factoring this into a shared helper/base class to reduce duplication and keep future behavior changes consistent across backends.
- The STIR adaptor’s `_extract_voxel_size_mm` falls back to `get_grid_spacing()[3]` and the SIRF adaptor only supports `voxel_sizes()[2]`; it might be worth aligning these conventions and documenting/validating expected units/axis ordering to avoid subtle geometry mismatches between backends.
## Individual Comments
### Comment 1
<location path="sirf_simind_connection/connectors/stir_adaptor.py" line_range="153-155" />
<code_context>
+ if len(voxel_sizes) >= 3:
+ return float(voxel_sizes[2])
+ if hasattr(image, "get_grid_spacing"):
+ spacing = image.get_grid_spacing()
+ try:
+ return float(spacing[3])
+ except Exception as exc: # pragma: no cover - backend-specific
+ raise ValueError(
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential off‑by‑one when reading voxel spacing from get_grid_spacing()
With a 3-element spacing tuple, indexing `spacing[3]` will always raise IndexError and force the error path, so `get_grid_spacing()` can never succeed. If you intended to use the z-spacing, this should be `spacing[2]`, or the expected spacing format should be clarified in documentation.
</issue_to_address>
### Comment 2
<location path="tests/test_pytomography_adaptor.py" line_range="61-70" />
<code_context>
- monkeypatch.setattr(adapter_mod, "AcquisitionDataInterface", DummyInterface)
-
-
-@pytest.mark.unit
-def test_backend_adapter_prefers_first_backend(monkeypatch):
- """First wrapped object sets backend preference."""
</code_context>
<issue_to_address>
**suggestion (testing):** Add positive tests for new PyTomographySimindAdaptor geometry + connector wiring, not only interface-removal checks.
The current test only checks removal of `build_system_matrix` / `get_pytomography_metadata`, so it doesn’t validate the new adaptor–connector behavior.
Please add a positive test that, via a monkeypatched `SimindPythonConnector`, verifies that:
- a small `(x, y, z)` source and mu-map tensor passed through the adaptor reaches `configure_voxel_phantom` in SIMIND `(z, y, x)` order,
- `voxel_size_mm`, energy windows, and `scoring_routine` are forwarded correctly via `set_energy_windows` / `run`, and
- `get_total_output` / `get_scatter_output` / `get_penetrate_output` simply return the connector’s outputs.
This will exercise the new connector-first integration and axis handling end-to-end, not just the removed methods.
</issue_to_address>
### Comment 3
<location path="tests/test_python_connector.py" line_range="218-227" />
<code_context>
- monkeypatch.setattr(adapter_mod, "AcquisitionDataInterface", DummyInterface)
-
-
-@pytest.mark.unit
-def test_backend_adapter_prefers_first_backend(monkeypatch):
- """First wrapped object sets backend preference."""
</code_context>
<issue_to_address>
**suggestion (testing):** Extend SimindPythonConnector.configure_voxel_phantom tests to cover validation and attenuation edge cases.
These tests are a solid start. To better cover `configure_voxel_phantom`, consider adding cases for:
1. Non-3D inputs: pass a 2D or 4D `source_array`/`mu_map_array` and assert `ValueError` with the "must both be 3D arrays" message.
2. Non-positive `voxel_size_mm`: call with 0 or negative and assert `ValueError("voxel_size_mm must be > 0")`.
3. Attenuation disabled: with the attenuation flag off and a nonzero `mu_map`, assert `density_u16` in the written file is all zeros.
4. Enum `scoring_routine`: pass a `ScoringRoutine` enum and assert `cfg.get_value(84) == routine.value`.
These additions would strengthen regression coverage for this helper.
</issue_to_address>
### Comment 4
<location path="tests/test_connectors_base.py" line_range="14-23" />
<code_context>
- monkeypatch.setattr(adapter_mod, "AcquisitionDataInterface", DummyInterface)
-
-
-@pytest.mark.unit
-def test_backend_adapter_prefers_first_backend(monkeypatch):
- """First wrapped object sets backend preference."""
</code_context>
<issue_to_address>
**suggestion (testing):** Add behavioural tests for the new STIR/SIRF adaptor implementations (validation, voxel size extraction, and runtime path).
Currently this file only checks the inheritance hierarchy and that `quantization_scale` stays an `__init__` parameter. The refactor added substantial behaviour in `SirfSimindAdaptor` / `StirSimindAdaptor` (input validation, conversion to/from NumPy, voxel size extraction, and output handling) that isn’t covered.
Please add targeted behavioural tests using small dummy STIR/SIRF-like objects and a monkeypatched `SimindPythonConnector` that records `configure_voxel_phantom` calls. Suggested assertions:
- `run()` raises `ValueError` when source or mu_map is missing, or shapes differ.
- `run()` forwards float32 arrays with the expected shape and `scoring_routine` into `configure_voxel_phantom`.
- `_extract_voxel_size_mm` uses `voxel_sizes()` or `get_grid_spacing()` and raises `ValueError` when neither is present.
- `get_penetrate_output` / `_get_component` raise `KeyError` listing available keys when a component is missing.
This will exercise the new adaptor behaviour rather than just its signature.
</issue_to_address>
### Comment 5
<location path="sirf_simind_connection/core/executor.py" line_range="62" />
<code_context>
subprocess.run(command, check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Summary
Validation
Summary by Sourcery
Refactor the SIMIND integration around a connector-first core, routing all adaptors through the NumPy-based SimindPythonConnector and retiring the legacy simulator stack.
Enhancements:
Build:
Documentation:
Tests:
Chores: