GetEstimatedValues is not thread-safe for k-nearest neighbours model

[gkronber]

r14236: added synchronization in NearestNeighbourModel and made some related changes to GradientBoostedTreesModelSurrogate

[mkommend]

Reviewed r14236.

• I dislike locking on an exposed objects (kdTree) that is modified by some alglib method calls. A private locking object in the NearestNeighbourModel and GBTModelSurrogate would be more appropriate.

• GBTSurrogateModel should use a double checked locking pattern [0] for handling null values. AFAIK lock on a null reference has no effect.

   lock (actualModel) { if (actualModel == null) actualModel = RecalculateModel(); }
  

  The use of a Lazy<T> for initialization of the actualModel would be suitable and simplify / remove all the scattered locks.

[0] ​https://en.wikipedia.org/wiki/Double-checked_locking

[gkronber]

r14314: added a separate lock object for the kdTree in NearestNeighbourModel

[gkronber]

r14315: using Lazy initialization pattern to remove explicit locking (happens within Lazy instead)

[mkommend]

r14322: Made lock object readonly in NearestNeighbourModel.

[mkommend]

Btw, I always thought that we use American English for developing, because all the NearestNeighbour classes are written with an U, whereas all other classes containing Neighbor are written differently.

Not a big deal, but annoying to search for.

[mkommend]

Reviewed r14314 & r14315.

The following changesets must be released: 14236, 14314, 14315 and 14322

[gkronber]

Michael, thank you for your careful review.

Replying to mkommend:

Btw, I always thought that we use American English for developing, because all the NearestNeighbour classes are written with an U, whereas all other classes containing Neighbor are written differently.

Yes, this is inconsistent, sorry for that. I guess we have to live with some imperfections in HeuristicLab.

[gkronber]

r14327: merged r14236,r14314:14315 and r14322 from trunk to stable