search

BAMresearch / McSAS3

Refactored McSAS for analysis of X-ray and neutron scattering data
https://bamresearch.github.io/McSAS3/
GNU General Public License v3.0
3 stars 4 forks source link

Add an option to define the number of threads/processes used for optimization #4

Closed MarcBHahn closed 2 years ago

MarcBHahn commented 2 years ago

It would be useful to have an option included which allows to define the number of parallel threads/processes to be used. e.g.:

mcsas3_cli_runner.py -t n

with n is equal to the number of parallel threads. Whereby n=-1 could be the maximum number of threads available on the system.

MarcBHahn commented 2 years ago

Implemented the changes in my fork and the branch https://github.com/MarcBHahn/McSAS3/tree/threads-option. If you want me to merge it, let me know. Includes a fix for https://github.com/BAMresearch/McSAS3/issues/5.

The option can be used as: "-n", "--nThreads", type=int, default=0, help="The number (n>0) of cores/threads used for optimization. If omitted, the value from the config file is used (default). Never more threads are used as cores exist.",

toqduj commented 2 years ago

This option is already available in the run configuration file. There, you can specify the nCores parameter to limit the maximum number of parallel threads. .. I'll have to take a look at how you did it to see if it's not conflicting or duplicating this functionality...

toqduj commented 2 years ago

I see how you did it, you wanted a command-line option to override the number of cores in the configuration, which makes sense from a usability perspective.

I think it will not work as it is, though, for example with the special case of nThreads =1, as the nUserThreads check on line 110 is too late. Also, I suspect it might get confusing to have more core and thread parameters in McHat's "self" than necessary.

For cleanliness, I would include the processing of this input argument in mcsas_cli_runner, and overwrite the nCores key/value pair in the read optDict there if present. (around line 65). No need to pass it on to McHat.

Would you mind making this change? Thanks in advance!

MarcBHahn commented 2 years ago

You are right, its somewhat duplicating, and to use it either as an argument or within the config file is more a question of personal taste.

MarcBHahn commented 2 years ago

Would you mind making this change? Thanks in advance!

Yes, I will as soon as I find some time.

toqduj commented 2 years ago

Functionality already exists, closing issue.