-
Notifications
You must be signed in to change notification settings - Fork 13
Optimize #1002 merge single multi pld quantification #1307
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync a bit further
Nice, but it is only a small step further to make this function really multi- and single-PLD ignorant as much as possible, and only at the end do a check for dependencies.
This will help with the generalizability, and readability of our code. In many instances, you want the same for multi-PLD and single-PLD, or it is something that won't matter for single-PLD (like sorting or choosing which PLD or LabDur to use).
A dependency could be:
warning: multiple PLDs detected but singlePLD quantification requested, using max PLD instead, this may give suboptimal results
and later, we could replace this by:
warning: multiple PLDs detected but singlePLD quantification requested, using multiPLD quantification instead
OR
warning: multiple PLDs detected but singlePLD quantification requested, now performing both using multiPLD and singlePLD quantifications
Only quantification
Also, you didn't do the second requirement in the issue list, you only merged the quantification right? I changed the title of the issue & PR accordingly, and we can make a new issue for merging this in other locations (or do we really not have multi-PLD specific processing elsewhere yet? Then we should make that a new issue, if we don't have it already ;) Anyway, in that case the title change would still be good, but my comment can otherwise be ignored
Please pull
I did a small spelling correction & layout for readability, and I didn't want to annoy you by asking this so I did it myself :)
Reviewers
Bea doesn't need to review this right?
Agree with this philosophy in general. And parts of the code should be like that. If not, please make specific comments or propose new issues. I would prefer to do only the basics here.
Yes - but see above.
Yes - I only merged stuff.
Perfect - that makes things more efficient. And I anyway check all new commits for errors.
Review code itself not, but test yes :D Hence the review request. |
HenkMutsaerts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I did a minor revamp of xASL_quant_ASL
Looks all good. But Bea still needs to run it and test on both single/multi-PLD and Hadamard. |
BeatrizPadrela
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for:
- single-PLD (Philips_PCASL_2DEPI_3.2.1.1_1)
- multi-PLD (Philips_PCASL_2DEPI_3.2.1.1_dummyMultiPLD_1)
- BBB-ASL (DZNE subject data)
d921623 to
9662951
Compare

Linked issue
Close #1002