Skip to content
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

Speed improvement by changing convolution code #29

Merged
merged 3 commits into from Jun 4, 2017
Merged

Speed improvement by changing convolution code #29

merged 3 commits into from Jun 4, 2017

Conversation

notZaki
Copy link
Contributor

@notZaki notZaki commented Jun 4, 2017

There's two changes in this pull request.

The main change is that the convolution in the Tofts-Key Model is now performed using the algorithm detailed in the appendix of:
Flouri, Lesnic, & Sourbron (2016). MRM 76(3), doi: 10.1002/mrm.25991.
The new implementation provides a considerable improvement in speed without substantially changing the results - if anything, the errors are slightly smaller in noiseless QIBA 6 data.

At the bottom, I've included the summary statistics to compare the old version and the new convolution implementation.
The big change is the runtime which was calculated by running the fit 6 times, and then taking the mean +/- stdDev of the runtime from the last five fits - the first runtime is discarded because it is generally slower than the rest.

The second change is minor and involves commenting out one line which was causing validate(6) to fail. Commenting it out resolved the problem and validate(6) now runs successfully for me on Julia 0.5.2.
There is still a problem with validate(4) - I think it's the lines involving 'xticks' and maybe also 'yticks'.
I'm not sure if this is a general problem or if it's just a problem on my end, so I didn't touch validate(4).

Comparison between Old and New convolution implementation

1. QIBA 6 - Noiseless
(1321 x 30 points on one CPU core)

Term Old New
Kt RMSE 0.42 0.06
Kt max 2.17 0.14
Kt CCC 0.99993 0.99999
ve RMSE 0.13 0.08
ve max 0.57 0.23
ve CCC 0.999999374 0.999999376
Runtime[s] 26.6 +/- 0.2 0.36 +/- 0.09

2. QIBA 6 - Noisy
(1321 x 3000 points on one CPU core)

Term New
Kt RMSE 21.4
Kt max 100
Kt CCC 0.8508
ve RMSE 16.06
ve max 100
ve CCC 0.8707
Runtime[s] 26.2
  • This takes much longer using the 'old' method, so I skipped it. I'm assuming you have values from a previous validation run, which can be compared against these values.

3. QIBA 4 - Noiseless
(661 x 90 points on one CPU core)

Term Old New
Kt RMSE 6.974653262 6.974660651
Kt max 23.49363809 23.49362108
Kt CCC 0.999800984 0.999800811
ve RMSE 18.02170569 18.02171367
ve max 100 100
ve CCC 0.890429062 0.890429128
vp RMSE 23.77019514 23.80389152
vp max 92.10582798 92.5412823
vp CCC 0.999920099 0.999919898
Runtime[s] 20.9 +/- 0.2 0.5

@coveralls
Copy link

coveralls commented Jun 4, 2017

Coverage Status

Changes Unknown when pulling 7bec04d on notZaki:NewConvolution into ** on davidssmith:master**.

@coveralls
Copy link

coveralls commented Jun 4, 2017

Coverage Status

Changes Unknown when pulling 183ffe2 on notZaki:NewConvolution into ** on davidssmith:master**.

@notZaki
Copy link
Contributor Author

notZaki commented Jun 4, 2017

Update: Managed to get validate(4) to work by using 'collect' instead of the old square-bracket method for concatenating numbers. This was previously fixed in pull request #16, but the fix was applied only to validate(6).

With these changes, I can now run validate() in Julia 0.5.2 without any errors - and all of the validation trials take roughly a minute on a single core!

@davidssmith davidssmith merged commit adc427e into davidssmith:master Jun 4, 2017
@davidssmith
Copy link
Owner

Amazing work. Thanks a ton! I checked everything myself this morning, and it works great on v0.5. There is some work to make it compatible with v0.6, but not much I think.

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.

None yet

3 participants