Enforce nthreads = 1 for single threaded lib

[lu1and10]

Enforce nthreads = 1 for single threaded lib, to improve the case that single threaded finufft lib running with nthreads != 1 gives inf/nan error flatironinstitute/jax-finufft#73 and #430.

[dfm]

I'm looking at this now too, and another option would be to use the same approach as the one in spreadinterp.cpp, avoiding the extra ifdef:

finufft/src/spreadinterp.cpp

Lines 269 to 271 in 9744d14

	int maxnthr = MY_OMP_GET_MAX_THREADS();
	if (opts.nthreads>0) // user override up to max avail
	maxnthr = min(maxnthr,opts.nthreads);

I don't have any argument for why one would be better than the other, but could be good to be consistent!

Edited to confirm that this does seem to fix flatironinstitute/jax-finufft#73.

[lu1and10]

I'm looking at this now too, and another option would be to use the same approach as the one in spreadinterp.cpp, avoiding the extra ifdef:

finufft/src/spreadinterp.cpp

Lines 269 to 271 in 9744d14

	int maxnthr = MY_OMP_GET_MAX_THREADS();
	if (opts.nthreads>0) // user override up to max avail
	maxnthr = min(maxnthr,opts.nthreads);

I don't have any argument for why one would be better than the other, but could be good to be consistent!

Edited to confirm that this does seem to fix flatironinstitute/jax-finufft#73.

Do you mean these lines https://github.com/lu1and10/finufft/blob/9744d14b07b2d25bb9e5326d549bb1d7971a7f88/src/spreadinterp.cpp#L269-L271
right, it's better consistent. we can do that always cap num threads with MY_OMP_GET_MAX_THREADS(), I guess we should still warn user that user's nthreads input is not used in case nthr = min(nthr,p->opts.nthreads) chooses nthr not opts.nthreads?

[lu1and10]

the change here does not cap nthreads to omp_get_max_threads if omp is enabled, this is the same as the original code

finufft/src/finufft.cpp

Line 596 in 9744d14

nthr = p->opts.nthreads; // user override (no limit or check)

.
It only cap nthreads to 1 if omp is not enabled. @dfm suggests we could be consistent with spreader as in

finufft/src/spreadinterp.cpp

Lines 269 to 271 in 9744d14

	int maxnthr = MY_OMP_GET_MAX_THREADS();
	if (opts.nthreads>0) // user override up to max avail
	maxnthr = min(maxnthr,opts.nthreads);

to always cap nthreads to omp_get_max_threads for both omp enabled or not. @ahbarnett is there a reason the current code does not cap nthreads to omp_get_max_threads? It seems that nthreads also controls the num of threads for fft.

[blackwer]

My vote is for consistency. Always cap threads at the requested thread count. If it's single a single threaded build: 1, otherwise opts.nthreads. I'm ambivalent about the warning, but it seems reasonable.

[lu1and10]

My vote is for consistency. Always cap threads at the requested thread count. If it's single a single threaded build: 1, otherwise opts.nthreads. I'm ambivalent about the warning, but it seems reasonable.

I see, let's see what @ahbarnett 's thought about whether to use omp_get_max_threads or user requested threads count to cap nthreads.

The warning could be better, English is not my native.... Concise and pretty warning message is appreciated.

[ahbarnett]

Thanks for parsing the logic here. I agree we should cap to omp_get_max_threads() and that being set to 1 indicates single-thread mode without invoking #ifdefs. So I like Dan's idea of following what I did in spreadinterp, but adding the warning. I'm going to try and push to your PR, Libin.

[lu1and10]

Thanks for parsing the logic here. I agree we should cap to omp_get_max_threads() and that being set to 1 indicates single-thread mode without invoking #ifdefs. So I like Dan's idea of following what I did in spreadinterp, but adding the warning. I'm going to try and push to your PR, Libin.

Thanks @ahbarnett, not sure you can push to my fork, added you to the fork collaborator, maybe we should use the official finufft repo creating a new branch for PR later on.

[ahbarnett]

This was good and respected the "unlimited thread override" the user could do. But based on Libin + Dan + Robert suggestions, I changed it to always limit to maxnthr given by OMP ( = 1 if single-threaded build, as per defs.h header), matching spreadinterp, and preventing JAX crashes.

[ahbarnett]

Happy for Libin etc to pull in, assuming CI good. Thanks!

[ahbarnett]

I forgot to update CHANGELOG and docs. Will do it in master. We should always do this for changes to behavior (what if a user relied on setting nthr > get_max_omp_threads() ?)