Skip to content

Optimize #971 get mean control#977

Merged
BeatrizPadrela merged 7 commits intodevelopfrom
optimize-#971_GetMeanControl
Dec 8, 2021
Merged

Optimize #971 get mean control#977
BeatrizPadrela merged 7 commits intodevelopfrom
optimize-#971_GetMeanControl

Conversation

@BeatrizPadrela
Copy link
Contributor

@BeatrizPadrela BeatrizPadrela commented Dec 8, 2021

Linked issue

Should work alreayd for Hadamard and for the other data formats

@BeatrizPadrela BeatrizPadrela changed the base branch from main to develop December 8, 2021 09:38
@MichaelStritt MichaelStritt linked an issue Dec 8, 2021 that may be closed by this pull request
xASL_im_GetMeanControl #971
Closed
4 tasks
@MichaelStritt MichaelStritt added the optimization Ensure that code runs faster with unchanged functionality label Dec 8, 2021
MichaelStritt
MichaelStritt reviewed Dec 8, 2021
View reviewed changes
@MichaelStritt
Copy link
Contributor

MichaelStritt commented Dec 8, 2021

On the first look I can't see any hard errors. If you say you already tested it on a few flavors then I'm fine, because we'll re-test everything for the release really soon anyway. Let's wait for Jan's feedback though 👍

MichaelStritt
MichaelStritt reviewed Dec 8, 2021
View reviewed changes
MichaelStritt
MichaelStritt approved these changes Dec 8, 2021
View reviewed changes
Copy link
Contributor

@MichaelStritt MichaelStritt left a comment

Choose a reason for hiding this comment

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

Let's wait for Jan's approval 👍

@jan-petr
Copy link
Contributor

jan-petr commented Dec 8, 2021

Some comments: NumberOfAverages might not be reliable for Hadamard, so we should really calculate the size of each Hadamard blockSize = MatrixSize*numberTEs

Then we also don't need to precalculate all the other sizes as we can then index the imASLTimeSeries directly with 1:blockSize:end

@BeatrizPadrela
Copy link
Contributor Author

BeatrizPadrela commented Dec 8, 2021

Some comments: NumberOfAverages might not be reliable for Hadamard, so we should really calculate the size of each Hadamard blockSize = MatrixSize*numberTEs

Then we also don't need to precalculate all the other sizes as we can then index the imASLTimeSeries directly with 1:blockSize:end

Indeed! Looks great now

jan-petr
jan-petr approved these changes Dec 8, 2021
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

@BeatrizPadrela BeatrizPadrela force-pushed the optimize-#971_GetMeanControl branch from 59f9d47 to 40d2938 Compare December 8, 2021 11:02
@BeatrizPadrela BeatrizPadrela merged commit 40d2938 into develop Dec 8, 2021
@BeatrizPadrela BeatrizPadrela deleted the optimize-#971_GetMeanControl branch December 8, 2021 11:03
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

+1 more reviewer

@MichaelStritt MichaelStritt MichaelStritt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BeatrizPadrela BeatrizPadrela

Labels

optimization Ensure that code runs faster with unchanged functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

xASL_im_GetMeanControl

3 participants

@BeatrizPadrela @MichaelStritt @jan-petr