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
Fix multithreading issues in global tractography #824
Conversation
Threading issue likely originated from maintaining data structure integrity for random particle selection. Now removed list of pointers and shifted this functionality to ParticlePool, to avoid any accidental pointer invalidation.
I've tested extensively on my new linux system. @maxpietsch confirmed that this compiles on macOS. Do I wait for someone to check if it compiles on windows too or do we just merge it in hope for the best? |
Give me a minute, I'll give it a crack on my system... |
Perfect, thanks. (Fingers crossed) |
OK, compiles fine on Windows 10. Haven't actually tried running the command, do you need me to do that? It'll be a while before I find the time. This may be a good time to add some tests...? 😉 |
Good, thanks for that. I mainly wanted to make sure it compiles cross-platform before merging. I did 3 runs, each with 1 billion iterations on 20 threads, all successful (only took half an hour by the way). In these MCMC algorithms, anything that can go wrong will eventually happen. So I'm fairly confident that no more segfaults or race conditions occur in this version. Even if a user does find something, this version is definitely an improvement over the current master, so I think we can hit merge. I would also like to have tests in TravisCI, but I don't know how to deal with the random nature of the algorithm. Even if I allow to fix the seed for the random generators, there are quite a few of them and their interaction is quite complicated. |
I've merged these changes. I'll think about adding tests when I find some time. |
These fixes should address issue #477 and the segfault that occurred on some systems. I think this is now ready to be merged into master, although it might be good to test on windows and other systems too.