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

freqanalysis: use parfor instead of for #2097

Merged
merged 12 commits into from Sep 22, 2022

Conversation

AljenU
Copy link

@AljenU AljenU commented Aug 19, 2022

For this function, using parfor does appear to be useful. The timing information gathered on some testfunctions does show improvement (#1853 (comment)), and this is also confirmed by the profiling reports.

This function also shows that sometimes, quite some work is needed to enable changing a for loop to a parfor loop.

Note that for now only the keeprpt cases 1 and 2 have a parfor loop, the case 4 is still a regular for loop. This because the case 4 indexes into the output in a way that is not compatible with parfor. Refactoring that case to use parfor could also be done, but it would use a lot more temporary memory.

move fixed-result out of the loop
move only-once inside the main if itrial==1 check
To prepare for doing it out of the loop, to enable making the loop a parfor instead of for.
Instead of looking at the call stack, and expecting ft_freqanalysis at a
specific position, have an explicit option in the specest functions on
whether ft_progress can be used.
The variables acttimboind and acttboi have (for quite some time now) the same content, so no need to calculate twice
To prepare for using parfor for the main loop, need to have a different loop for
each keeprpt option, because the outputs are asssigned to differently, which is not
allowed within a parfor loop.
Setting this limit to 0 will make the execution non-parallel
@robertoostenveld
Copy link
Contributor

robertoostenveld commented Sep 21, 2022

Hi @AljenU,

Following this, can you change the branch for this pull request following this documentation. It should point to the parallel branch, which ar this moment is still in perfect sync with the master branch.

And can you do the same for PR #2068 and #2069 ? I will then merge them all.

thanks,
Robert

@AljenU AljenU changed the base branch from master to parallel September 22, 2022 07:52
@AljenU
Copy link
Author

AljenU commented Sep 22, 2022

Hi @AljenU,

Following this, can you change the branch for this pull request following this documentation. It should point to the parallel branch, which ar this moment is still in perfect sync with the master branch.

And can you do the same for PR #2068 and #2069 ? I will then merge them all.

thanks, Robert

@robertoostenveld Done

@robertoostenveld robertoostenveld merged commit fd0f556 into fieldtrip:parallel Sep 22, 2022
@robertoostenveld
Copy link
Contributor

thanks!

@robertoostenveld
Copy link
Contributor

There are still a few things on https://github.com/fieldtrip/fieldtrip/projects/5 to be done by us (i.e. @schoffelen and me), mainly regarding documentation and updating the README on the parallel branch.

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

2 participants