COMP: apply split between threads and work units in ITK5 #10
Conversation
| @@ -154,7 +154,7 @@ void | |||
| LabelSetMorphBaseImageFilter< TInputImage, doDilate, TOutputImage > | |||
| ::GenerateData(void) | |||
| { | |||
| ThreadIdType nbthreads = this->GetNumberOfThreads(); | |||
| ThreadIdType nbthreads = this->GetNumberOfWorkUnits(); | |||
There was a problem hiding this comment.
We should also change the variable name: nbthreads.
There was a problem hiding this comment.
I had noticed it at various points in ITK itself and the remotes as well. Since the name of the variable is not consistent across all of them, this would require a non-negligible amount of work. May be an issue in ITK would help having this in mind. If no objections exist, I'll open the issue tonight, and the desired variable naming can be discussed there.
There was a problem hiding this comment.
Good idea to address it in other places.
To avoid confusion, workUnits or numberOfWorkUnits will be more accurate than a variable that indicates it is the number of threads.
There was a problem hiding this comment.
I like pieces. It is short, and its use can be inferred from the method names it is used with.
There was a problem hiding this comment.
I do like pieces also: can remain unchanged when at some point in time multi-threading gets a new refactoring (not in the near future we hope 😁 ).
There was a problem hiding this comment.
nbthreads is not an English word, I may suggest something like numberOfThreads (already used in ITK), this would also help keep tool like codespell happy.
To get an idea, here are some stats from ITK master:
$ ack numberOfThreads | wc -l
179
$ ack nbthreads | wc -l
7
There was a problem hiding this comment.
Thanks for the stats Jean-Christophe: opened in InsightSoftwareConsortium/ITK#86
No description provided.