Skip to content

Warning gantry and couch angles + correct displayed progress #879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lisaseckler wants to merge 1 commit into
base: dev
Choose a base branch
from
+29 −20
Open

Warning gantry and couch angles + correct displayed progress #879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions matRad/steering/matRad_StfGeneratorExternalRayBixelAbstract.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ function setDefaults(this)
nBeams = numel(this.gantryAngles);
if nBeams ~= numel(this.couchAngles)
matRad_cfg = MatRad_Config.instance();
matRad_cfg.dispWarning('For some reason, we have a different number of beam and couch angles!');
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon at the end of this statement. In MATLAB, statements without semicolons print their output to the console, which would display the error message twice (once from dispError and once from the return value).

Suggested change
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.');

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is inaccurate. This condition triggers when the number of gantry angles doesn't equal the number of couch angles, not specifically when there are "more gantry angles than couch angles". The message should be more generic to cover both cases (e.g., "The number of gantry angles and couch angles must be equal.").

Suggested change
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
matRad_cfg.dispError('The number of gantry angles and couch angles must be equal.')

Copilot uses AI. Check for mistakes.
end
end

Expand All @@ -67,9 +67,11 @@ function setDefaults(this)
if ~this.lockAngleUpdate
this.lockAngleUpdate = true;
if numel(this.gantryAngles) > numel(this.couchAngles)
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon at the end of this statement. In MATLAB, statements without semicolons print their output to the console, which would display the error message twice (once from dispError and once from the return value).

Suggested change
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.');

Copilot uses AI. Check for mistakes.
%Append Couch angles with zeros
this.couchAngles = [this.couchAngles zeros(1,numel(this.gantryAngles)-numel(this.couchAngles))];
elseif numel(this.couchAngles) > numel(this.gantryAngles)
Comment on lines 69 to 73
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matRad_cfg variable is used but not defined in this scope. It needs to be instantiated with matRad_cfg = MatRad_Config.instance(); before calling dispError.

Suggested change
if numel(this.gantryAngles) > numel(this.couchAngles)
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
%Append Couch angles with zeros
this.couchAngles = [this.couchAngles zeros(1,numel(this.gantryAngles)-numel(this.couchAngles))];
elseif numel(this.couchAngles) > numel(this.gantryAngles)
if numel(this.gantryAngles) > numel(this.couchAngles)
matRad_cfg = MatRad_Config.instance();
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
%Append Couch angles with zeros
this.couchAngles = [this.couchAngles zeros(1,numel(this.gantryAngles)-numel(this.couchAngles))];
elseif numel(this.couchAngles) > numel(this.gantryAngles)
matRad_cfg = MatRad_Config.instance();

Copilot uses AI. Check for mistakes.
Comment on lines 69 to 73
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matRad_cfg variable is used but not defined in this scope. It needs to be instantiated with matRad_cfg = MatRad_Config.instance(); before calling dispError.

Suggested change
if numel(this.gantryAngles) > numel(this.couchAngles)
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
%Append Couch angles with zeros
this.couchAngles = [this.couchAngles zeros(1,numel(this.gantryAngles)-numel(this.couchAngles))];
elseif numel(this.couchAngles) > numel(this.gantryAngles)
if numel(this.gantryAngles) > numel(this.couchAngles)
matRad_cfg = MatRad_Config.instance();
matRad_cfg.dispError('There are more gantry angles than couch angles. They have to have the same size.')
%Append Couch angles with zeros
this.couchAngles = [this.couchAngles zeros(1,numel(this.gantryAngles)-numel(this.couchAngles))];
elseif numel(this.couchAngles) > numel(this.gantryAngles)
matRad_cfg = MatRad_Config.instance();

Copilot uses AI. Check for mistakes.
matRad_cfg.dispError('There are more couch angles than gantry angles. They have to have the same size.')
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing semicolon at the end of this statement. In MATLAB, statements without semicolons print their output to the console, which would display the error message twice (once from dispError and once from the return value).

Suggested change
matRad_cfg.dispError('There are more couch angles than gantry angles. They have to have the same size.')
matRad_cfg.dispError('There are more couch angles than gantry angles. They have to have the same size.');

Copilot uses AI. Check for mistakes.
%Try to identify the removed beam angles
[removedAngles,ix] = setdiff(oldAngles,this.gantryAngles);

Expand Down Expand Up @@ -239,7 +241,7 @@ function initialize(this)

% Show progress
if matRad_cfg.logLevel > 2
matRad_progress(i,length(this.gantryAngles));
matRad_progress(i,length(this.gantryAngles),1);
end

%% visualization
Expand Down Expand Up @@ -384,7 +386,7 @@ function initialize(this)

% Show progress
if matRad_cfg.logLevel > 2
matRad_progress(i,length(this.gantryAngles));
matRad_progress(i,length(this.gantryAngles),2);
Copy link
Contributor

@remocristoforetti remocristoforetti Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this second call to matRad_progress

end
end
end
Expand Down
41 changes: 24 additions & 17 deletions matRad/util/matRad_progress.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
function matRad_progress(currentIndex, totalNumberOfEvaluations)
function matRad_progress(currentIndex, totalNumberOfEvaluations,scen)
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear variable naming. The parameter name scen is ambiguous - it's not clear what "scen" stands for (scenario? scene?). Based on usage, it appears to control progress display mode. Consider a more descriptive name like progressMode, displayMode, or updateType.

Copilot uses AI. Check for mistakes.
% matRad progress bar
%
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new scen parameter breaks backward compatibility with all existing calls to matRad_progress throughout the codebase. Many files call this function with only 2 parameters (see matRad_sampling.m, matRad_DoseEngineBase.m, etc.). Consider making scen an optional parameter with a default value, e.g., function matRad_progress(currentIndex, totalNumberOfEvaluations, scen) with logic like if nargin < 3, scen = 1; end.

Suggested change
%
%
if nargin < 3
scen = 1;
end

Copilot uses AI. Check for mistakes.
% call
Expand Down Expand Up @@ -27,23 +27,30 @@ function matRad_progress(currentIndex, totalNumberOfEvaluations)
% LICENSE file.
%
% %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

% If it's not the first step, erase the stuff printed before
if (currentIndex == 1)
fprintf('Progress: ');

persistent isInitialized
Copy link
Contributor

@remocristoforetti remocristoforetti Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid the use of persistent variables for this as matRad_progress is also called outside of the stf generator. Also seems like it's not used.


if isempty(isInitialized)
isInitialized = true;
end

if (currentIndex > 1)
Length = numel(sprintf('%3.2f %%',(currentIndex-1)/totalNumberOfEvaluations*100));
fprintf(repmat('\b',1,Length));

percent = (currentIndex / totalNumberOfEvaluations) * 100;

% --- first part of the progress ---
if scen == 1
Copy link
Contributor

@remocristoforetti remocristoforetti Dec 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we need a scen input, and seems like the output is never cleared, keeps adding the 'Progress '

% new line
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment "new line" is misleading. The \r escape sequence is a carriage return that moves the cursor to the beginning of the current line, not a new line. If a new line is actually needed, use \n instead. If the intent is to overwrite the current line (which seems correct given the context), update the comment to reflect this, e.g., "overwrite current line".

Suggested change
% new line
% overwrite current line

Copilot uses AI. Check for mistakes.
fprintf('\rProgress: %6.2f %%', percent);
end

% Print the progress tool
fprintf('%3.2f %%',currentIndex/totalNumberOfEvaluations*100);

% After the last iteration print a newline command
if (currentIndex == totalNumberOfEvaluations)
fprintf('\n');

% --- for scenario 2, same line ---
if scen == 2
fprintf(' Progress: %6.2f %%', percent);
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The progress display for scen == 2 will append " Progress: X.XX %" to whatever is currently on the console line without proper updating. This will create messy output if called multiple times. Consider using backspace characters (\b) to erase the previous output, or use \r to return to the beginning of the line and overwrite, similar to how scen == 1 works.

Suggested change
fprintf(' Progress: %6.2f %%', percent);
fprintf('\rProgress: %6.2f %%', percent);

Copilot uses AI. Check for mistakes.
end


% --- finish line after the last angle ---
if currentIndex == totalNumberOfEvaluations && scen == 2
fprintf('\n');
clear isInitialized
Comment on lines +31 to +53
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The persistent isInitialized variable is declared and set but never actually used to control any logic. It's only cleared when currentIndex == totalNumberOfEvaluations && scen == 2, but this clearing has no effect since the variable isn't checked anywhere. Consider removing this unused code or implementing the intended initialization logic.

Copilot uses AI. Check for mistakes.
end


Loading