Range shifter positioning for MC and minor bug fixing by remocristoforetti · Pull Request #880 · e0404/matRad

remocristoforetti · 2025-12-01T08:51:16Z

Minor bug fixes:

IMPT stf generator
Hong PB algorithm, call to matRad_calcSigmaRashi
MCsquare: handle presence of range shifter, need and additional "field" in the MC parameter files for this

…oxel belongs to target

… first/last voxel

wahln · 2025-12-01T08:53:34Z

@JenHardt could you please do the review first and then I will take a look

Copilot

Pull request overview

This pull request implements range shifter positioning for Monte Carlo dose calculation engines and addresses several minor bugs in the IMPT steering file generator and Hong pencilbeam algorithm. The changes enable proper handling of range shifters in MCsquare simulations by splitting fields based on range shifter presence and correcting positioning calculations.

Key Changes:

Fixed range shifter positioning calculations in MCsquare and TOPAS MC engines
Corrected matRad_calcSigmaRashi function calls to pass the full base data entry instead of just energy
Added handling for edge cases in IMPT generator where targets start/end at voxel boundaries
Fixed matRad_getIsoCenter to properly calculate mean along rows

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
test/steering/test_stfGEneratorParticleIMPT.m	Added test case for range shifter generation in IMPT
test/doseCalc/test_sigmaRashi.m	New test for sigma calculation with range shifter
test/doseCalc/test_HongPB.m	Added test for Hong PB dose calculation with range shifter
matRad/steering/matRad_StfGeneratorParticleSingleBeamlet.m	Fixed undefined `visBool` variable by adding as class property
matRad/steering/matRad_StfGeneratorParticleRayBixelAbstract.m	Added `rangeShifterEqD` property and logic to handle user-defined thickness
matRad/steering/matRad_StfGeneratorParticleIMPT.m	Fixed target entry/exit detection for edge cases and range shifter references
matRad/geometry/matRad_getIsoCenter.m	Fixed mean calculation to compute along dimension 1
matRad/doseCalc/+DoseEngines/matRad_TopasMCEngine.m	Corrected range shifter position by adding half-width offset
matRad/doseCalc/+DoseEngines/matRad_ParticlePencilBeamEngineAbstract.m	Updated `matRad_calcSigmaRashi` calls to pass full baseData entry
matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m	Implemented field splitting for range shifter handling and corrected positioning formula
matRad/basedata/matRad_MCemittanceBaseData.m	Changed access modifier to make `getRangeShiftersFromStf` public

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-01T09:00:25Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m

+                            isoToRaShi = stf(i).SAD - rangeShifter.sourceRashiDistance - rsWidth;
+                            fprintf(fileHandle,'####IsocenterToRangeShifterDistance\n%f\n',isoToRaShi/10); %in cm


[nitpick] The sign change in this calculation should be verified. Previously it was + rsWidth, now it's - rsWidth. Combined with the sign change in line 815 (removing the negative), this represents a significant change in the range shifter positioning calculation. Please ensure this has been validated with test data, as incorrect positioning could lead to dose calculation errors.

Suggested change

isoToRaShi = stf(i).SAD - rangeShifter.sourceRashiDistance - rsWidth;

fprintf(fileHandle,'####IsocenterToRangeShifterDistance\n%f\n',isoToRaShi/10); %in cm

isoToRaShi = stf(i).SAD - rangeShifter.sourceRashiDistance + rsWidth;

fprintf(fileHandle,'####IsocenterToRangeShifterDistance\n%f\n',-isoToRaShi/10); %in cm

I guess this is because of the reported range shifter misalignment in #877 ?

Copilot · 2025-12-01T09:00:26Z

matRad/basedata/matRad_MCemittanceBaseData.m

    end

-    methods (Access = protected)
+    methods %(Access = protected)


The commented-out access modifier changes the visibility of the getRangeShiftersFromStf method from protected to public. This should either have the comment removed (if making it public is intentional) or be reverted to Access = protected. Leaving it commented out is unclear and appears incomplete.

Suggested change

methods %(Access = protected)

methods (Access = protected)

Is there actually a reason why this is commented? Could it be deleted?

Copilot · 2025-12-01T09:00:26Z

test/steering/test_stfGEneratorParticleIMPT.m

Typo in function name: "test_generateRangeShfterStf" should be "test_generateRangeShifterStf" (missing 'i' in 'Shifter').

Suggested change

function test_generateRangeShfterStf()

function test_generateRangeShifterStf()

Copilot · 2025-12-01T09:00:27Z

matRad/steering/matRad_StfGeneratorParticleRayBixelAbstract.m

Missing semicolon after property declaration. All other property declarations in this file and MATLAB conventions typically use semicolons to suppress output.

Suggested change

useRangeShifter = false

useRangeShifter = false;

Copilot · 2025-12-01T09:00:27Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m

Typo in comment: "Selecte" should be "Select" (extra 'e' at the end).

Suggested change

% Selecte the energies that have a RaShi for

% Select the energies that have a RaShi for

Copilot · 2025-12-01T09:00:28Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m

+                                    % If more than one energy layer is
+                                    % found, one of them is for the
+                                    % RaShiField
+                                    energyIx = energyIx([stf(i).ray(j).rangeShifter(energyIx).ID] == 0); % Select the one with no RaShi;


Potential bug: When filtering energyIx on line 391, if all energies for this layer have a range shifter (i.e., none have ID == 0), energyIx will become empty. This would cause issues when used on lines 394, 401, 404, and 406. Consider adding a check to skip processing if energyIx is empty after filtering, or ensure this case is handled properly.

Copilot · 2025-12-01T09:00:28Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m

+                            raShiLayers = [];
+                            for j = 1:stf(i).numOfRays
+                                currentRay = stf(i).ray(j);
+                                raShiLayers = [raShiLayers, currentRay.energy([currentRay.rangeShifter.ID] == stfFieldMCsquareRaShi.rangeShifterID)];
+                            end
+                            stfFieldMCsquareRaShi.energies = unique(raShiLayers);


[nitpick] Inconsistent naming: The variable is named raShiLayers (plural) but represents energy values, not layers. Consider renaming to raShiEnergies to be consistent with line 189 where a similar concept uses the name raShiEnergies.

Suggested change

raShiLayers = [];

for j = 1:stf(i).numOfRays

currentRay = stf(i).ray(j);

raShiLayers = [raShiLayers, currentRay.energy([currentRay.rangeShifter.ID] == stfFieldMCsquareRaShi.rangeShifterID)];

end

stfFieldMCsquareRaShi.energies = unique(raShiLayers);

raShiEnergies = [];

for j = 1:stf(i).numOfRays

currentRay = stf(i).ray(j);

raShiEnergies = [raShiEnergies, currentRay.energy([currentRay.rangeShifter.ID] == stfFieldMCsquareRaShi.rangeShifterID)];

end

stfFieldMCsquareRaShi.energies = unique(raShiEnergies);

Copilot · 2025-12-01T09:00:28Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m

Typo in comment: "compuation" should be "computation" (missing 't').

Suggested change

%Number of primaries depending on beamlet-wise or field-based compuation (direct dose calculation)

%Number of primaries depending on beamlet-wise or field-based computation (direct dose calculation)

Copilot · 2025-12-01T09:00:29Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m

+                                % we can eliminate the layer from the
+                                % non-RaShi field
+                                if isscalar(raShiIDSinLayer) && raShiIDSinLayer == stfFieldMCsquareRaShi.rangeShifterID
+                                    stfFieldMCsquare(stfFieldMCsquare.energies == layerEnergy) = [];


Potential bug: The variable stfFieldMCsquare is being indexed like an array on line 349 (stfFieldMCsquare(stfFieldMCsquare.energies == layerEnergy) = []), but it was initialized as a struct on line 283, not as an array. This should likely be stfFieldMCsquare.energies(stfFieldMCsquare.energies == layerEnergy) = [] to remove specific energy values from the energies array.

Suggested change

stfFieldMCsquare(stfFieldMCsquare.energies == layerEnergy) = [];

stfFieldMCsquare.energies(stfFieldMCsquare.energies == layerEnergy) = [];

Copilot · 2025-12-01T09:00:29Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m

Command injection risk: mcSquareCall is constructed via string concatenation with untrusted data (this.mcSquareBinary) and executed with system, allowing shell metacharacters in the binary path to be interpreted. If an attacker can influence this.mcSquareBinary (e.g., via configuration), they can inject ;, &&, or similar to execute arbitrary commands. Fix by avoiding the shell entirely (e.g., use Java ProcessBuilder or .NET System.Diagnostics.Process to pass executable and args separately), or at minimum strictly whitelist/validate this.mcSquareBinary and quote/escape both the binary and argument:

% Safer (still shell-based): ensure both elements are quoted and validated cmd = sprintf('"%s" "%s"', this.mcSquareBinary, MCsquareConfigFile); [status, cmdout] = system(cmd); % Preferred: Java ProcessBuilder without shell pb = java.lang.ProcessBuilder(this.mcSquareBinary, MCsquareConfigFile); proc = pb.start();

codecov · 2025-12-01T09:03:56Z

Codecov Report

❌ Patch coverage is 65.68047% with 58 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.02%. Comparing base (5a7d07b) to head (5e6a53c).
⚠️ Report is 4 commits behind head on dev.

Files with missing lines	Patch %	Lines
...eCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m	66.43%	48 Missing ⚠️
matRad/steering/matRad_StfGeneratorParticleIMPT.m	64.28%	5 Missing ⚠️
...teering/matRad_StfGeneratorParticleSingleBeamlet.m	0.00%	2 Missing ⚠️
...eEngines/matRad_ParticlePencilBeamEngineAbstract.m	66.66%	1 Missing ⚠️
...atRad/doseCalc/+DoseEngines/matRad_TopasMCEngine.m	0.00%	1 Missing ⚠️
...ring/matRad_StfGeneratorParticleRayBixelAbstract.m	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #880      +/-   ##
==========================================
+ Coverage   53.85%   54.02%   +0.16%     
==========================================
  Files         308      308              
  Lines       19938    20011      +73     
==========================================
+ Hits        10737    10810      +73     
  Misses       9201     9201

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2025-12-01T09:21:20Z

Test Results

3 files ± 0 3 suites ±0 44m 0s ⏱️ +13s
323 tests + 4 323 ✅ + 4 0 💤 ±0 0 ❌ ±0
1 065 runs +12 1 060 ✅ +12 5 💤 ±0 0 ❌ ±0

Results for commit 5e6a53c. ± Comparison against base commit 5a7d07b.

♻️ This comment has been updated with latest results.

JenHardt

looks good to me, please change the spelling and the semicolon copilot suggested

wahln · 2025-12-19T14:57:48Z

maybe we could also use this opportunity to write a range shifter test for MCsquare ?

github-actions · 2026-01-19T01:48:13Z

This PR was automatically marked as stale it has been open 30 days with no activity. Please review/update/merge this PR.

…hifterMCFix

wahln

Nice work, but please answer to the copilot comments and address my remaining ones from this review

wahln · 2026-02-03T12:04:25Z

matRad/steering/matRad_StfGeneratorParticleRayBixelAbstract.m

+                % availablePeaPos for indexing
+                this.availablePeakPosRaShi(this.availablePeakPosRaShi<0) = 0;
+
+                matRad_cfg.dispWarning('Use of range shifter enabled. matRad will generate a generic range shifter with WEPL %f to enable ranges below the shortest base data entry.',this.rangeShifterEqD);


This warning now only makes sense if the rangeShifterEqD is not provided, right?
I think we need to messages (and it does not need to be a warning)

Using provided range shfiter thickness of %f mm

Using generic range shifter thickness of %f mm determined to allow ranges below the shortest base data entry.

wahln · 2026-02-03T12:06:15Z

matRad/basedata/matRad_MCemittanceBaseData.m

    end

-    methods (Access = protected)
+    methods %(Access = protected)


Is there actually a reason why this is commented? Could it be deleted?

wahln · 2026-02-03T12:06:57Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m


            matRad_cfg = MatRad_Config.instance();

+            if isfolder(this.externalCalculation)


What is the problem here?
also you don't need "else end"

wahln · 2026-02-03T12:09:33Z

matRad/doseCalc/+DoseEngines/matRad_ParticleMCsquareEngine.m

+                            isoToRaShi = stf(i).SAD - rangeShifter.sourceRashiDistance - rsWidth;
+                            fprintf(fileHandle,'####IsocenterToRangeShifterDistance\n%f\n',isoToRaShi/10); %in cm


I guess this is because of the reported range shifter misalignment in #877 ?

wahln · 2026-02-03T12:10:12Z

matRad/steering/matRad_StfGeneratorParticleIMPT.m

+                                        matRad_cfg.dispWarning('Target exit on the last voxel');
+                                        exitIx = numel(rho{shiftScen}{1});
+                                    else
+                                        diff_voi    = [diff([rho{shiftScen}{end}])];


diff_voi does not align with camelCase naming convention actually.

wahln · 2026-02-03T12:11:16Z

matRad/steering/matRad_StfGeneratorParticleSingleBeamlet.m

    properties 
        energy;
        raShiThickness = 50; %Range shifter to be used if useRangeShifter = true;
+        visBool = false;


I don't like "visBool". Alternative suggestions? "visualize"?

wahln · 2026-02-03T12:11:59Z

test/doseCalc/test_MCsquareEngine.m

Does this also test with range shifter? I don't see it.

remocristoforetti added 14 commits November 30, 2025 18:26

Minor bug fix: correct call to matRad_getIsocenter

2b513f3

Minor bug fix: explicit direction for mean(), effective if only one v…

786c0f4

…oxel belongs to target

Bug fix: add rangeShifterEq thickness as property

d095e94

Allow user to set rangeShifterEqThickness

d4ba549

Bug fix: handle of target entry/exit point when taregt begins/ends on…

e2f9e8e

… first/last voxel

Bug fix: get equivalent thickness of range shifter

c8b6743

Big fix: call to matRad_calcSigmaRashi

4436e6a

Allow access to getRangeShiftersFromStf

be84504

Bug fix: handling of range shifters

9814d26

Allow for special characters in filenames for system call

924c44b

Big fix: Move position of range shifter

238b83b

Add tests for range shfter generation and calculation

7a223f1

Add test for matRad_calcSigmaRashi

b05a836

Bug fix: range shifter field handling

1834041

remocristoforetti requested review from JenHardt and wahln December 1, 2025 08:51

wahln requested a review from Copilot December 1, 2025 08:53

Copilot started reviewing on behalf of wahln December 1, 2025 08:53 View session

Copilot finished reviewing on behalf of wahln December 1, 2025 08:55

Copilot AI reviewed Dec 1, 2025

View reviewed changes

remocristoforetti added 2 commits December 1, 2025 11:06

Remove plotting from raShi test

856730b

Update use of round functon for Octave compatibility

8bc7fd9

JenHardt reviewed Dec 2, 2025

View reviewed changes

github-actions bot added the stale Automatic label for stale issues label Jan 19, 2026

Minor typo fixes; add externalCalculation option for MCsquare engine

7390a02

remocristoforetti added 3 commits January 30, 2026 11:44

Remove read external option for MCsquare

aeb01c6

Octave compatibility for MCsquare test

9790a96

Octave test compatibility

550b64a

github-actions bot removed the stale Automatic label for stale issues label Jan 31, 2026

Merge branch 'dev' of https://github.com/e0404/matRad into dev_rangeS…

5e6a53c

…hifterMCFix

wahln requested changes Feb 3, 2026

View reviewed changes

		isoToRaShi = stf(i).SAD - rangeShifter.sourceRashiDistance - rsWidth;
		fprintf(fileHandle,'####IsocenterToRangeShifterDistance\n%f\n',isoToRaShi/10); %in cm

	function test_generateRangeShfterStf()
	function test_generateRangeShifterStf()

	% Selecte the energies that have a RaShi for
	% Select the energies that have a RaShi for

	%Number of primaries depending on beamlet-wise or field-based compuation (direct dose calculation)
	%Number of primaries depending on beamlet-wise or field-based computation (direct dose calculation)

	stfFieldMCsquare(stfFieldMCsquare.energies == layerEnergy) = [];
	stfFieldMCsquare.energies(stfFieldMCsquare.energies == layerEnergy) = [];


		matRad_cfg = MatRad_Config.instance();

		if isfolder(this.externalCalculation)

Conversation

remocristoforetti commented Dec 1, 2025

Uh oh!

wahln commented Dec 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

JenHardt left a comment

Choose a reason for hiding this comment

Uh oh!

wahln commented Dec 19, 2025

Uh oh!

github-actions bot commented Jan 19, 2026

Uh oh!

wahln left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov bot commented Dec 1, 2025 •

edited

Loading

github-actions bot commented Dec 1, 2025 •

edited

Loading