Skip to content

Optimization #997 improve pwi4 d#1527

Merged
jan-petr merged 96 commits intofeature-#1551_RevampedSubtractionTempBranchfrom
optimization-#997_Improve_PWI4D
Jan 4, 2024
Merged

Optimization #997 improve pwi4 d#1527
jan-petr merged 96 commits intofeature-#1551_RevampedSubtractionTempBranchfrom
optimization-#997_Improve_PWI4D

Conversation

@HenkMutsaerts
Copy link
Member

@HenkMutsaerts HenkMutsaerts commented Oct 26, 2023 •
edited by jan-petr
Loading

Linked issue

Temporary check by Jan for closing #997 in the future

@HenkMutsaerts HenkMutsaerts linked an issue Oct 26, 2023 that may be closed by this pull request
Revamp PWI4D behavior #997
Closed
28 tasks
jan-petr
jan-petr requested changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

Essentially good. but some errors need to be fixed.

@jan-petr jan-petr self-assigned this Oct 27, 2023
HenkMutsaerts
HenkMutsaerts commented Oct 31, 2023
View reviewed changes
jan-petr
jan-petr requested changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

A few more comments.

HenkMutsaerts
HenkMutsaerts commented Nov 2, 2023
View reviewed changes
jan-petr
jan-petr requested changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

A new batch of comments.

jan-petr
jan-petr requested changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

A second batch of comments.

jan-petr
jan-petr requested changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

A few more changes

jan-petr
jan-petr requested changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

A few more comments.

jan-petr
jan-petr requested changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

A new batch of comments and answers.

jan-petr
jan-petr requested changes Nov 22, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

More comments.

@HenkMutsaerts HenkMutsaerts force-pushed the optimization-#997_Improve_PWI4D branch from e7b073d to e79f32b Compare November 23, 2023 09:46
jan-petr
jan-petr requested changes Nov 23, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

Minor bugs.

jan-petr
jan-petr requested changes Nov 24, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

Small bugs

HenkMutsaerts
HenkMutsaerts commented Nov 29, 2023
View reviewed changes
HenkMutsaerts
HenkMutsaerts commented Nov 29, 2023
View reviewed changes
HenkMutsaerts
HenkMutsaerts commented Nov 29, 2023
View reviewed changes
HenkMutsaerts
HenkMutsaerts commented Nov 29, 2023
View reviewed changes
HenkMutsaerts
HenkMutsaerts commented Nov 29, 2023
View reviewed changes
HenkMutsaerts
HenkMutsaerts commented Nov 30, 2023
View reviewed changes
jan-petr
jan-petr requested changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

We can perhaps remove some of the comments in the code - as resolved. See details and if you agree.

HenkMutsaerts
HenkMutsaerts commented Dec 1, 2023
View reviewed changes
jan-petr
jan-petr reviewed Dec 1, 2023
View reviewed changes
jan-petr
jan-petr requested changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

More comments.

HenkMutsaerts
HenkMutsaerts commented Dec 1, 2023
View reviewed changes
Copy link
Member Author

@HenkMutsaerts HenkMutsaerts left a comment

Choose a reason for hiding this comment

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

Two minor questions, I will have a few more of these iterations, just so I start understanding it better, so I can add more comments, so it becomes also more understandable for somebody new to Hadamard/multi-TE. And to ascertain modularity/generalizability.

HenkMutsaerts
HenkMutsaerts commented Dec 5, 2023
View reviewed changes
Copy link
Member Author

@HenkMutsaerts HenkMutsaerts left a comment

Choose a reason for hiding this comment

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

Added a few suggestions to simplify the code/comments.

HenkMutsaerts
HenkMutsaerts commented Dec 13, 2023
View reviewed changes
Copy link
Member Author

@HenkMutsaerts HenkMutsaerts left a comment •
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 two bMultiTE parameters. To clarify:

I would keep single & multi-TE processing equal everywhere, except for only taking the first TE if bMultiTE is false.
And then quantification differs based on bMultiTE, like with bMultiPLD. So why 2 bMultiTE parameters?

bMultiTE should not be a substitute for numel(uniqueEchoTime)>1

So, as above, I suggest we keep:

bMultiTEQuantification & bMultiPLDQuantification

and remove bMultiTE and bMultiPLD.

Right?

jan-petr and others added 26 commits January 4, 2024 15:34
@jan-petr
#997 HadamardDecoding: Fix control4D calculation
eac5099
@jan-petr
#997 HadamardDecoding: Fix PWI4D calculation
aa32c15
@jan-petr
#997 Minor fixes
2e59b91
@jan-petr
#997 Separate Hadamard decode and interleave
c42cfd6
@jan-petr
#997 Restructure PLD vector calculation in subtraction
f14bd8d
@jan-petr
#997 quant_HadamardDecoding comments
bae06ce
@jan-petr
#997 Add setting for Multi-TE quantification
c238ef8
@jan-petr
#997 Fix simple subtraction parameter calculation
16e2e77
@jan-petr
#997 quant_HadamardDecoding: Improve names and comments
c20cef4
@jan-petr
#997 quant_HadamardDecoding: Improve further names and comments
3099837
@jan-petr
#997 quant_HadamardDecoding independent of EchoTimes
3ff6cdb
@HenkMutsaerts @jan-petr
#997 xASL_quant_HadamardDecoding: improving clarity
63a9d7c
@jan-petr
#997 quant_HadamardDecoding Fix comments
e6e0ac9
@jan-petr
#997 quant_HadamardDecoding: Minor bug fix
4e768be
@HenkMutsaerts @jan-petr
#997 xASL_quant_HadamardDecoding: clarifications
c7a3e75
@jan-petr
#997 quant_HadamardDecoding: Fix comments
ca47411
@HenkMutsaerts @jan-petr
#997 xASL_quant_HadamardDecoding: clarifications 2
1a70e11
@jan-petr
#997 quant_HadamardDecoding fix comments
335de91
@jan-petr
#997 quant_HadamardDecoding fix comments 2
947823d
@HenkMutsaerts @jan-petr
#997 xASL_quant_HadamardDecoding: clarifications 3
7d388e9
@jan-petr
#997 quant_HadamardDecoding: Fix repetition calculation
ed32da6
@HenkMutsaerts @jan-petr
#997 xASL_quant_HadamardDecoding: some more commentEdits
28ae164
@HenkMutsaerts @jan-petr
#997 xASL_im_ASLSubtractionAveraging: remove comment
0cdfe0d
@HenkMutsaerts @jan-petr
#997 xASL_im_ASLSubtractionAveraging: verify parameter length vs nVol…
a24de51 
…umes
@HenkMutsaerts @jan-petr
#997 xASL_wrp_RealignASL: exclude spikes in parameter vectors
4ca1da3
@jan-petr
#997 Update bQuantify parameters
10eb905
@jan-petr jan-petr force-pushed the optimization-#997_Improve_PWI4D branch from 0dad5b5 to 10eb905 Compare January 4, 2024 15:40
jan-petr
jan-petr approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

OK

@jan-petr jan-petr merged commit 10eb905 into feature-#1551_RevampedSubtractionTempBranch Jan 4, 2024
@jan-petr jan-petr deleted the optimization-#997_Improve_PWI4D branch January 4, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jan-petr jan-petr jan-petr approved these changes

Assignees

@jan-petr jan-petr

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Revamp PWI4D behavior

2 participants

@HenkMutsaerts @jan-petr