Skip to content

Conversation

@jan-petr
Copy link
Contributor

@jan-petr jan-petr commented Feb 1, 2023

Linked issue

Close #1002

@jan-petr jan-petr self-assigned this Feb 1, 2023
@jan-petr jan-petr linked an issue Feb 1, 2023 that may be closed by this pull request
2 tasks
Copy link
Member

@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.

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?

@HenkMutsaerts HenkMutsaerts changed the title Optimize #1002 merge single multi pld Optimize #1002 merge single multi pld quantification Feb 3, 2023
@HenkMutsaerts HenkMutsaerts removed the request for review from BeatrizPadrela February 3, 2023 05:54
@jan-petr
Copy link
Contributor Author

jan-petr commented Feb 7, 2023

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.

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.
For example, we should make a new issue that multi-TE single-PLD are correctly crunched - lets do that with the newly added flavor that you have.

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).
Here - the sorting will differ a bit necessarily. Not that it would have to be done differently in principle, but the implementation of the sorting for single and multi-PLD will slightly differ, so we need to account for it.

A dependency could be: warning: multiple PLDs detected but singlePLD quantification requested, using max PLD instead, this may give suboptimal results
There's never a mutli-PLD detected, but single-PLD requested - we don't have an input option for that. We can open a new issue and create such an option.

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

Yes - but see above.

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

Yes - I only merged stuff.

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 :)

Perfect - that makes things more efficient. And I anyway check all new commits for errors.

Reviewers

Bea doesn't need to review this right?

Review code itself not, but test yes :D Hence the review request.

Copy link
Member

@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.

Looks good, I did a minor revamp of xASL_quant_ASL

@jan-petr
Copy link
Contributor Author

jan-petr commented Feb 7, 2023

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.

Copy link
Contributor

@BeatrizPadrela BeatrizPadrela left a 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)

@BeatrizPadrela
Copy link
Contributor

All good after testing TestDataSets
image

@BeatrizPadrela BeatrizPadrela force-pushed the optimize-#1002_MergeSingleMultiPLD branch from d921623 to 9662951 Compare February 14, 2023 10:40
@BeatrizPadrela BeatrizPadrela merged commit 9662951 into develop Feb 14, 2023
@BeatrizPadrela BeatrizPadrela deleted the optimize-#1002_MergeSingleMultiPLD branch February 14, 2023 10:42
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.

Merge single- and multi-PLD quantification

4 participants