Add choice of type of attenuation/HU/density map for inpit#3
Open
samdporter wants to merge 3 commits intomainfrom
Open
Add choice of type of attenuation/HU/density map for inpit#3samdporter wants to merge 3 commits intomainfrom
samdporter wants to merge 3 commits intomainfrom
Conversation
Reviewer's GuideAdds a configurable mu_map_type to the SIMIND Python connector and its adaptors, allowing attenuation, density, or HU input maps to be converted to density consistently, and renames the distribution/documentation branding from py-smc to PySIMIND while updating tests and docs accordingly. Sequence diagram for mu_map_type flow from adaptor to density conversionsequenceDiagram
actor User
participant Adaptor as SirfSimindAdaptor
participant Connector as PythonConnector
participant Converter as _convert_mu_input_to_density
participant AttToDen as attenuation_to_density
participant HuToDen as hu_to_density_schneider
User->>Adaptor: __init__(mu_map_type)
Adaptor->>Adaptor: store _mu_map_type
User->>Adaptor: run(runtime_operator)
Adaptor->>Adaptor: prepare source and mu_map arrays
Adaptor->>Connector: configure_voxel_phantom(source, mu_map, voxel_size_mm, scoring_routine, mu_map_type=_mu_map_type)
Connector->>Connector: normalize mu_map_type
Connector->>Connector: validate mu_map_type in {attenuation, density, hu}
alt cfg flag 11 enabled
Connector->>Converter: _convert_mu_input_to_density(mu_map_array, mu_map_type, photon_energy)
alt mu_map_type == attenuation
Converter->>AttToDen: attenuation_to_density(mu_map_array, photon_energy)
AttToDen-->>Converter: density_g_cm3
else mu_map_type == density
Converter->>Converter: use mu_map_array as density_g_cm3
else mu_map_type == hu
Converter->>HuToDen: hu_to_density_schneider(mu_map_array)
HuToDen-->>Converter: density_g_cm3
end
Converter-->>Connector: density_mg_cm3
else cfg flag 11 disabled
Connector->>Connector: density = zeros_like(mu_map_array)
end
Connector-->>Adaptor: source_path, density_path
Adaptor->>Adaptor: continue simulation workflow
Class diagram for updated SIMIND connector mu_map_type handlingclassDiagram
class MuMapType {
<<typealias>>
attenuation
density
hu
}
class PythonConnector {
+configure_voxel_phantom(source, mu_map, voxel_size_mm, scoring_routine, mu_map_type)
+set_energy_windows(lower_bounds, upper_bounds, window_ids)
+run(runtime_operator)
+_convert_mu_input_to_density(mu_map_array, mu_map_type, photon_energy) np.ndarray
}
class PyTomographyAdaptor {
-python_connector PythonConnector
-_scoring_routine ScoringRoutine
-_mu_map_type MuMapType
-_source torch.Tensor
-_mu_map torch.Tensor
+__init__(python_connector, voxel_size_mm, quantization_scale, scoring_routine, mu_map_type)
+run(runtime_operator)
}
class SirfSimindAdaptor {
-python_connector PythonConnector
-_scoring_routine ScoringRoutine
-_mu_map_type MuMapType
-_source Any
-_mu_map Any
+__init__(python_connector, voxel_size_mm, photon_multiplier, quantization_scale, scoring_routine, mu_map_type)
+run(runtime_operator)
}
class StirSimindAdaptor {
-python_connector PythonConnector
-_scoring_routine ScoringRoutine
-_mu_map_type MuMapType
-_source Any
-_mu_map Any
+__init__(python_connector, voxel_size_mm, photon_multiplier, quantization_scale, scoring_routine, mu_map_type)
+run(runtime_operator)
}
MuMapType <.. PythonConnector : uses
MuMapType <.. PyTomographyAdaptor : stores
MuMapType <.. SirfSimindAdaptor : stores
MuMapType <.. StirSimindAdaptor : stores
PythonConnector <.. PyTomographyAdaptor : composition
PythonConnector <.. SirfSimindAdaptor : composition
PythonConnector <.. StirSimindAdaptor : composition
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 2 issues, and left some high level feedback:
- In
_convert_mu_input_to_density, relying on the prior normalization check and using a bareelsefor the HU case makes the function brittle to future changes; consider handling all threemu_map_typevalues explicitly and raising if an unexpected value is passed in. - The
normalized_map_typevariable is always astr, but_convert_mu_input_to_densityis typed to acceptMuMapType; aligning these types (or narrowingnormalized_map_typeafter validation) would make the type hints more accurate and help static analysis.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_convert_mu_input_to_density`, relying on the prior normalization check and using a bare `else` for the HU case makes the function brittle to future changes; consider handling all three `mu_map_type` values explicitly and raising if an unexpected value is passed in.
- The `normalized_map_type` variable is always a `str`, but `_convert_mu_input_to_density` is typed to accept `MuMapType`; aligning these types (or narrowing `normalized_map_type` after validation) would make the type hints more accurate and help static analysis.
## Individual Comments
### Comment 1
<location path="tests/test_python_connector.py" line_range="358-367" />
<code_context>
+ assert np.array_equal(captured["input"], hu_map)
+
+
+@pytest.mark.unit
+def test_python_connector_configure_voxel_phantom_rejects_unknown_mu_map_type(
+ tmp_path: Path,
+):
+ connector = SimindPythonConnector(
+ config_source=get("AnyScan.yaml"),
+ output_dir=tmp_path,
+ output_prefix="case01",
+ )
+ source = np.ones((3, 4, 5), dtype=np.float32)
+ mu_map = np.ones_like(source, dtype=np.float32)
+
+ with pytest.raises(ValueError, match="mu_map_type must be one of"):
+ connector.configure_voxel_phantom(
+ source=source,
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `mu_map_type` normalization (case and whitespace handling).
The implementation normalizes `mu_map_type` with `str(mu_map_type).strip().lower()`, but current tests only use already-lowercase, trimmed values. Please add cases that verify normalized inputs behave the same, for example:
- `mu_map_type=" HU "` behaving identically to `"hu"` (still using the HU map and converter).
- `mu_map_type=" Density "` behaving identically to `"density"`.
This will ensure the normalization behavior is covered by tests and protected against regressions.
Suggested implementation:
```python
density_u16 = np.fromfile(density_path, dtype=np.uint16)
assert density_u16.size == density_g_cm3.size
assert np.all(density_u16 == 1500)
@pytest.mark.unit
def test_python_connector_configure_voxel_phantom_normalizes_density_mu_map_type(
tmp_path: Path,
):
"""`mu_map_type` normalization (case/whitespace) should still use density handling."""
connector = SimindPythonConnector(
config_source=get("AnyScan.yaml"),
output_dir=tmp_path,
output_prefix="case01",
)
source = np.ones((3, 4, 5), dtype=np.float32)
density_g_cm3 = np.full_like(source, 1.5, dtype=np.float32)
# Intentionally use mixed case and surrounding whitespace
_, density_path = connector.configure_voxel_phantom(
source=source,
mu_map=density_g_cm3,
mu_map_type=" Density ",
)
density_u16 = np.fromfile(density_path, dtype=np.uint16)
assert density_u16.size == density_g_cm3.size
# Must behave identically to `mu_map_type="density"` (1.5 g/cm³ -> 1500)
assert np.all(density_u16 == 1500)
@pytest.mark.unit
```
To fully cover the suggestion for HU input, add a new test mirroring your existing HU-path test (the one that ends with `assert np.array_equal(captured["input"], hu_map)`), but call `configure_voxel_phantom` with `mu_map_type=" HU "` instead of `"hu"`. Reuse the same setup and capture mechanism (fixtures/monkeypatching) as that existing HU test and assert that the captured input is still identical to `hu_map`. This will validate that `mu_map_type` normalization (strip + lower) works for HU as well as density.
</issue_to_address>
### Comment 2
<location path="tests/test_native_adaptors.py" line_range="149" />
<code_context>
output_dir=str(tmp_path),
output_prefix="case01",
scoring_routine=ScoringRoutine.PENETRATE,
+ mu_map_type="density",
)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that verifies the adaptors forward the default `mu_map_type` of "attenuation".
Current tests cover explicit `mu_map_type` values ("density" and "hu"), but not the default. Please add a variant of the forward-wiring test (STIR/SIRF/pyTomography) that omits `mu_map_type` in the adaptor constructor and verifies that `configure_voxel_phantom` receives "attenuation". This guards against regressions in default forwarding.
Suggested implementation:
```python
output_dir=str(tmp_path),
output_prefix="case01",
scoring_routine=ScoringRoutine.PENETRATE,
**(
{"mu_map_type": mu_map_type}
if mu_map_type is not None
else {}
),
)
source_arr = np.arange(2 * 3 * 4, dtype=np.float64).reshape(2, 3, 4)
captured: dict[str, object] = {}
def fake_configure_voxel_phantom(
source, mu_map, voxel_size_mm, scoring_routine, mu_map_type
):
captured["source"] = source
captured["mu_map"] = mu_map
captured["mu_map_type"] = mu_map_type
```
I can only see a small part of the test, so the following updates are also required elsewhere in `tests/test_native_adaptors.py` to fully implement the requested coverage:
1. **Parameterize the existing forward-wiring test** (the one containing this snippet) so it runs with explicit and default `mu_map_type` values. For example, if the test is currently:
```python
def test_stir_adaptor_forwards_mu_map_type(...):
...
```
change it to:
```python
import pytest
@pytest.mark.parametrize(
"mu_map_type, expected_mu_map_type",
[
("density", "density"),
("hu", "hu"),
(None, "attenuation"), # default forwarding
],
)
def test_stir_adaptor_forwards_mu_map_type(mu_map_type, expected_mu_map_type, ...):
...
```
2. **Add an assertion that checks the forwarded `mu_map_type`**, using the value captured in `fake_configure_voxel_phantom`:
```python
assert captured["mu_map_type"] == expected_mu_map_type
```
3. **Repeat the same pattern for the SIRF and pyTomography adaptor tests** (their corresponding forward-wiring tests):
- Parameterize each with `mu_map_type` / `expected_mu_map_type` as above.
- Update their adaptor-construction calls to use the `**({...} if mu_map_type is not None else {})` pattern so that `mu_map_type` is omitted when `None`.
- Capture and assert `captured["mu_map_type"]` just like in the STIR test.
These changes ensure that:
- Explicit `"density"` and `"hu"` values are still tested.
- A new variant of each forward-wiring test runs **without** passing `mu_map_type`, and verifies `"attenuation"` is forwarded to `configure_voxel_phantom`, guarding against regressions in the default behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+358
to
+367
| @pytest.mark.unit | ||
| def test_python_connector_configure_voxel_phantom_accepts_density_input_type( | ||
| tmp_path: Path, | ||
| ): | ||
| connector = SimindPythonConnector( | ||
| config_source=get("AnyScan.yaml"), | ||
| output_dir=tmp_path, | ||
| output_prefix="case01", | ||
| ) | ||
| source = np.ones((3, 4, 5), dtype=np.float32) |
There was a problem hiding this comment.
suggestion (testing): Add tests for mu_map_type normalization (case and whitespace handling).
The implementation normalizes mu_map_type with str(mu_map_type).strip().lower(), but current tests only use already-lowercase, trimmed values. Please add cases that verify normalized inputs behave the same, for example:
mu_map_type=" HU "behaving identically to"hu"(still using the HU map and converter).mu_map_type=" Density "behaving identically to"density".
This will ensure the normalization behavior is covered by tests and protected against regressions.
Suggested implementation:
density_u16 = np.fromfile(density_path, dtype=np.uint16)
assert density_u16.size == density_g_cm3.size
assert np.all(density_u16 == 1500)
@pytest.mark.unit
def test_python_connector_configure_voxel_phantom_normalizes_density_mu_map_type(
tmp_path: Path,
):
"""`mu_map_type` normalization (case/whitespace) should still use density handling."""
connector = SimindPythonConnector(
config_source=get("AnyScan.yaml"),
output_dir=tmp_path,
output_prefix="case01",
)
source = np.ones((3, 4, 5), dtype=np.float32)
density_g_cm3 = np.full_like(source, 1.5, dtype=np.float32)
# Intentionally use mixed case and surrounding whitespace
_, density_path = connector.configure_voxel_phantom(
source=source,
mu_map=density_g_cm3,
mu_map_type=" Density ",
)
density_u16 = np.fromfile(density_path, dtype=np.uint16)
assert density_u16.size == density_g_cm3.size
# Must behave identically to `mu_map_type="density"` (1.5 g/cm³ -> 1500)
assert np.all(density_u16 == 1500)
@pytest.mark.unitTo fully cover the suggestion for HU input, add a new test mirroring your existing HU-path test (the one that ends with assert np.array_equal(captured["input"], hu_map)), but call configure_voxel_phantom with mu_map_type=" HU " instead of "hu". Reuse the same setup and capture mechanism (fixtures/monkeypatching) as that existing HU test and assert that the captured input is still identical to hu_map. This will validate that mu_map_type normalization (strip + lower) works for HU as well as density.
| output_dir=str(tmp_path), | ||
| output_prefix="case01", | ||
| scoring_routine=ScoringRoutine.PENETRATE, | ||
| mu_map_type="density", |
There was a problem hiding this comment.
suggestion (testing): Add a test that verifies the adaptors forward the default mu_map_type of "attenuation".
Current tests cover explicit mu_map_type values ("density" and "hu"), but not the default. Please add a variant of the forward-wiring test (STIR/SIRF/pyTomography) that omits mu_map_type in the adaptor constructor and verifies that configure_voxel_phantom receives "attenuation". This guards against regressions in default forwarding.
Suggested implementation:
output_dir=str(tmp_path),
output_prefix="case01",
scoring_routine=ScoringRoutine.PENETRATE,
**(
{"mu_map_type": mu_map_type}
if mu_map_type is not None
else {}
),
)
source_arr = np.arange(2 * 3 * 4, dtype=np.float64).reshape(2, 3, 4)
captured: dict[str, object] = {}
def fake_configure_voxel_phantom(
source, mu_map, voxel_size_mm, scoring_routine, mu_map_type
):
captured["source"] = source
captured["mu_map"] = mu_map
captured["mu_map_type"] = mu_map_typeI can only see a small part of the test, so the following updates are also required elsewhere in tests/test_native_adaptors.py to fully implement the requested coverage:
-
Parameterize the existing forward-wiring test (the one containing this snippet) so it runs with explicit and default
mu_map_typevalues. For example, if the test is currently:def test_stir_adaptor_forwards_mu_map_type(...): ...
change it to:
import pytest @pytest.mark.parametrize( "mu_map_type, expected_mu_map_type", [ ("density", "density"), ("hu", "hu"), (None, "attenuation"), # default forwarding ], ) def test_stir_adaptor_forwards_mu_map_type(mu_map_type, expected_mu_map_type, ...): ...
-
Add an assertion that checks the forwarded
mu_map_type, using the value captured infake_configure_voxel_phantom:assert captured["mu_map_type"] == expected_mu_map_type
-
Repeat the same pattern for the SIRF and pyTomography adaptor tests (their corresponding forward-wiring tests):
- Parameterize each with
mu_map_type/expected_mu_map_typeas above. - Update their adaptor-construction calls to use the
**({...} if mu_map_type is not None else {})pattern so thatmu_map_typeis omitted whenNone. - Capture and assert
captured["mu_map_type"]just like in the STIR test.
- Parameterize each with
These changes ensure that:
- Explicit
"density"and"hu"values are still tested. - A new variant of each forward-wiring test runs without passing
mu_map_type, and verifies"attenuation"is forwarded toconfigure_voxel_phantom, guarding against regressions in the default behavior.
There was a problem hiding this comment.
Pull request overview
Adds a new mu_map_type option across the connector and adaptor APIs to support supplying attenuation (cm⁻¹), density (g/cm³), or CT HU maps and converting them to SIMIND’s density input format; also updates project branding to “PySIMIND” and documents the new option.
Changes:
- Add
mu_map_typeparameter toSimindPythonConnector.configure_voxel_phantom()with HU→density (Schneider) and attenuation→density conversion paths. - Forward
mu_map_typethrough STIR/SIRF/PyTomography adaptors and extend tests accordingly. - Rebrand package/docs (
py-smc→PySIMIND) and documentmu_map_typeusage.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_pytomography_adaptor.py | Ensures adaptor forwards mu_map_type through to connector wiring. |
| tests/test_python_connector.py | Adds unit tests for density/HU inputs and invalid mu_map_type. |
| tests/test_native_adaptors.py | Verifies STIR/SIRF adaptors forward mu_map_type. |
| sirf_simind_connection/connectors/stir_adaptor.py | Adds mu_map_type option and forwards it into configure_voxel_phantom(). |
| sirf_simind_connection/connectors/sirf_adaptor.py | Adds mu_map_type option and forwards it into configure_voxel_phantom(). |
| sirf_simind_connection/connectors/pytomography_adaptor.py | Adds mu_map_type option and forwards it into configure_voxel_phantom(). |
| sirf_simind_connection/connectors/python_connector.py | Implements mu_map_type parsing/validation and conversion helper. |
| sirf_simind_connection/init.py | Updates branding/version lookup distribution names. |
| pyproject.toml | Renames distribution to PySIMIND. |
| docs/usage.rst | Documents mu_map_type and shows connector/adaptor examples. |
| docs/testing.rst | Updates branding mention. |
| docs/intro.rst | Updates branding mention. |
| docs/index.rst | Updates documentation title branding. |
| docs/conf.py | Updates Sphinx project name and version lookup distribution names. |
| docs/changelog.rst | Changelog entry for mu_map_type and branding changes. |
| README.md | Updates branding, install command, and documents mu_map_type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+18
to
+21
| from sirf_simind_connection.converters.attenuation import ( | ||
| attenuation_to_density, | ||
| hu_to_density_schneider, | ||
| ) |
There was a problem hiding this comment.
HU conversion relies on hu_to_density_schneider(), which reads Schneider2000.json from package resources. Currently pyproject.toml only includes sirf_simind_connection/data/*.atn as package data, so installed distributions may miss Schneider2000.json and HU workflows will fail at runtime. Include Schneider2000.json (or data/*.json) in package data / manifest so the resource is shipped with the wheel/sdist.
Comment on lines
+135
to
+139
| normalized_map_type = str(mu_map_type).strip().lower() | ||
| if normalized_map_type not in {"attenuation", "density", "hu"}: | ||
| raise ValueError( | ||
| "mu_map_type must be one of: 'attenuation', 'density', 'hu'" | ||
| ) |
There was a problem hiding this comment.
normalized_map_type is computed as a plain str and then passed into _convert_mu_input_to_density(), which is typed to accept MuMapType (a Literal[...]). This is a type-hint mismatch that will trip static type checkers and makes the intent less clear. After validating membership, cast normalized_map_type to MuMapType (or change _convert_mu_input_to_density to accept str) so the types align.
| scoring_routine: SIMIND scoring routine identifier. | ||
| mu_map_type: Interpretation of ``mu_map`` values: | ||
| - ``"attenuation"``: linear attenuation (cm^-1), converted to density. | ||
| - ``"density"``: density (g/cm^3), written directly. |
There was a problem hiding this comment.
The docstring for mu_map_type="density" says the density map is "written directly", but the implementation still scales by * 1000.0 and rounds/clips to uint16 (mg/cm^3 in the SIMIND file). Please clarify in the docstring (and/or user docs) that the input is expected in g/cm^3 but the written file uses mg/cm^3 scaling, to avoid users passing already-mg values and getting a 1000× error.
Suggested change
| - ``"density"``: density (g/cm^3), written directly. | |
| - ``"density"``: density (g/cm^3); values are expected in g/cm^3 and | |
| are internally scaled to mg/cm^3 and quantized to ``uint16`` when | |
| writing the SIMIND density file. |
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.
New feature:
Can now input an attenuation map, CT, or density map into the connector (or adaptors) and this will be automatically converted to denity for SIMIND,
Summary by Sourcery
Add support for selecting attenuation, density, or HU input types for voxel phantoms and propagate this option through adaptors while renaming the package to PySIMIND in metadata and docs.
New Features:
Enhancements:
Build:
Documentation:
Tests: