ImgUtil.copy for two RandomAccessibleInterval is missing the "n_threads" parameter

[acardona]

The code here:

imglib2/src/main/java/net/imglib2/util/ImgUtil.java

Line 318 in 7e37c9b

public static < T extends Type< T >> void copy( final RandomAccessibleInterval< T > source, final RandomAccessibleInterval< T > destination )

... is very clean and simple, but it's missing a critical parameter: the number of threads to use.

A second ImgUtil.copy method would offer an additional n_threads parameter.

The new LoopBuilder has a multiThreaded method used in ImgUtil.copy but it takes a TaskExecutor obtained from the Parallelization.getTaskExecutor(). While Parallelization has methods to run an Action multithreaded, it is not obvious how to ask the Parallelization for a TaskExecutor that uses a predetermined number of threads.

Fortunately, the TaskExecutors class has a numThreads( int numThreads ) method that returns a TaskExecutor.

So the new method could look like this:

/**
	 * Copy one image into another, multi-threaded.
	 */
	public static < T extends Type< T >> void copy( final RandomAccessibleInterval< T > source, final RandomAccessibleInterval< T > destination, final int n_threads )
	{
		LoopBuilder.setImages(source, destination)
				.multiThreaded( TaskExecutors.numThreads( n_threads ) )
				.forEachPixel( (i,o) -> o.set(i) );
	}

Any objections to adding this method, given that the current replacement ImgUtils.copy falls short of its original implementation with the n_threads parameter?

[tpietzsch]

I think this goes a bit against the idea of the Parallelization framework, which is to if possible avoid passing explicit numThreads or ExecutorService parameters to algorithms.

In your code, the TaskExecutor should be closed, after it is no longer needed.
You could use Parallelization.runWithNumThreads() to do that automatically:

public static < T extends Type< T >> void copy( final RandomAccessibleInterval< T > source, final RandomAccessibleInterval< T > destination, final int n_threads )
{
	Parallelization.runWithNumThreads( n_threads, () -> copy( source, destination ) );
}

At this point, to me it seems to not add enough to justify adding a new method.
In particular, because it is then tempting for users to write

copy( source1, destination1, 64 );
copy( source2, destination2, 64 );
copy( source3, destination3, 64 );
copy( source4, destination4, 64 );

which would spin up a new ThreadPool every time instead of

Parallelization.runWithNumThreads( 64, () -> {
	copy( source2, destination2 );
	copy( source1, destination1 );
	copy( source3, destination3 );
	copy( source4, destination4 );
} );

which would reuse the same one.

If you are concerned about the lambdas in a scripting language setting, there is also Parallelization.setExecutorRequiresReset()

imglib2/src/main/java/net/imglib2/parallel/Parallelization.java

Lines 225 to 256 in 7e37c9b

	* This method sets the {@link TaskExecutor} for the current thread.
	* This can be used to execute a certain part of your code with the given
	* {@link TaskExecutor}. It's mandatory to call the {@code close()} of the
	* return {@link Frame frame} afterwards. This could be done using an
	* try-with-resources statement.
	* <pre>
	* {@code
	*
	* try ( Parallelization.Frame frame = Parallelization.setExecutorRequiresReset( taskExecutor ) )
	* {
	* myAlgorithm(input);
	* }
	* }
	* </pre>
	* Or by explicitly calling {@code frame.close()} in the finally block.
	* <pre>
	* {@code
	*
	* Parallelization.Frame frame = Parallelization.setExecutorRequiresReset( taskExecutor );
	* try
	* {
	* myAlgorithm(input);
	* }
	* finally
	* {
	* frame.close();
	* }
	* }
	* </pre>
	*/
	// NB: package-private to allow testing
	static Frame setExecutorRequiresReset( TaskExecutor taskExecutor )

@maarzt Opinions?