Skip to content

Class based Sequencing#891

Open
JenHardt wants to merge 25 commits intoe0404:devfrom
JenHardt:dev_Sequencing
Open

Class based Sequencing#891
JenHardt wants to merge 25 commits intoe0404:devfrom
JenHardt:dev_Sequencing

Conversation

@JenHardt
Copy link
Copy Markdown
Contributor

Implementation of a class to handle the photon sequencing with the three algorithims siochi, xia, engle as well as an ion sequencer

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 59.22250% with 493 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.77%. Comparing base (bf8adb3) to head (5b6e8a8).
⚠️ Report is 2 commits behind head on dev.

Files with missing lines Patch % Lines
matRad/gui/widgets/matRad_WorkflowWidget.m 16.43% 183 Missing ⚠️
...ring/matRad_StfGeneratorExternalRayBixelAbstract.m 38.93% 69 Missing ⚠️
matRad/sequencing/matRad_PhotonSequencerAbstract.m 70.40% 66 Missing ⚠️
matRad/sequencing/matRad_SequencerBase.m 58.33% 55 Missing ⚠️
...atRad/sequencing/matRad_SequencingPhotonsXiaLeaf.m 73.68% 20 Missing ⚠️
...ad/sequencing/matRad_SequencingPhotonsSiochiLeaf.m 88.42% 14 Missing ⚠️
...Rad/sequencing/matRad_SequencingPhotonsEngelLeaf.m 88.88% 12 Missing ⚠️
matRad/4D/matRad_acc4dDose.m 47.61% 11 Missing ⚠️
matRad/4D/matRad_calc4dDose.m 60.00% 10 Missing ⚠️
matRad/sequencing/matRad_engelLeafSequencing.m 0.00% 10 Missing ⚠️
... and 9 more
Additional details and impacted files
@@            Coverage Diff             @@
##             dev     #891       +/-   ##
==========================================
+ Coverage   0.00%   54.77%   +54.77%     
==========================================
  Files        310      317        +7     
  Lines      20289    20458      +169     
==========================================
+ Hits           0    11205    +11205     
+ Misses     20289     9253    -11036     

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

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 27, 2026

Test Results

    3 files      3 suites   43m 33s ⏱️
  354 tests   354 ✅ 0 💤 0 ❌
1 158 runs  1 153 ✅ 5 💤 0 ❌

Results for commit 5b6e8a8.

♻️ This comment has been updated with latest results.

@wahln
Copy link
Copy Markdown
Contributor

wahln commented Feb 27, 2026

There seem to be a bunch of unhappy tests in this PR ;-)

@wahln
Copy link
Copy Markdown
Contributor

wahln commented Feb 27, 2026

Also I am confused about the changed 4D capabilities (that don't seem to have anything to do with sequencing in parts).
Also, we should keep downwards compatibility with the old sequencing calls for a while.

Copy link
Copy Markdown
Contributor

@wahln wahln left a comment

Choose a reason for hiding this comment

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

Did some preliminary rough review.

@@ -1,4 +1,4 @@
function resultGUI = matRad_sequencing(resultGUI,stf,dij,pln,visBool)
function resultGUI = matRad_sequencing(resultGUI,stf,pln,dij,visMode)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason why the dij has become optional? This is a huge change without much benefit it seems.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is just because the dij is just used to recalculate the dose based on the new sequenced weight vector, but you could run the sequencing independently

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could probably still be removed?

@@ -1,29 +1,23 @@
function [resultGUI, timeSequence] = matRad_calc4dDose(ct, pln, dij, stf, cst, resultGUI, totalPhaseMatrix,accType)
function resultGUI = matRad_calc4dDose(dij, pln,stf,resultGUI, totalPhaseMatrix)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why did the argument list chagne so drastically?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the ct was only used to save the number of ct scenarios and the motion period , this should now be in the pln scenario model and the cst was used for the ax and bx vectors this is taken directly from the dij

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

okay let's approach it from the other way: can we avoid the pln struct when using the ct?
I would like to keep pln out of lower-level functions as much as possible, and only pass it to top-level workflow functions (as the pln can, for example, be easily changed in the GUI and lead to inconsistent inputs).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i think this can be done, but then i need the ct again,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no acctually i need it to selecect the sequencer and calculate the phaseMatrix if not given, although right now the phaseMatrix is only calculated for ions not photons

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn#t this a function that should actually be moved into the IonSequencer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is this was one of the files i deleted, where i just now reverted the delition to make it backwards compatible, i haved pushed the update to this file jet

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change in the interface destroys downwards compatibility.. Is this necessary?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i removed it because it was never used in the function

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Naming the file sequencingOld and the function still sequencing will create problems. I suggest to not change the interface of the sequencing call (with the optional dij at the end) but just keep the old interface (resultGUI,stf,dij,pln,visBool) -> visBool can become visMode, this is still OK just selecting the visMode 1 if it is true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

similar to above this is an issue of the delition and reverting of the delition that is not completet yet

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is changed in the submodule? Is this intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants