Github user chouqin commented on a diff in the pull request: https://github.com/apache/spark/pull/2435#discussion_r17765427 --- Diff: mllib/src/main/scala/org/apache/spark/mllib/tree/impl/DTStatsAggregator.scala --- @@ -189,6 +160,230 @@ private[tree] class DTStatsAggregator( } this } +} + +/** + * DecisionTree statistics aggregator. + * This holds a flat array of statistics for a set of (nodes, features, bins) + * and helps with indexing. + * + * This instance of [[DTStatsAggregator]] is used when not subsampling features. + * + * @param numNodes Number of nodes to collect statistics for. + */ +private[tree] class DTStatsAggregatorFixedFeatures( + metadata: DecisionTreeMetadata, + numNodes: Int) extends DTStatsAggregator(metadata) { + + /** + * Offset for each feature for calculating indices into the [[_allStats]] array. + * Mapping: featureIndex --> offset + */ + private val featureOffsets: Array[Int] = { + metadata.numBins.scanLeft(0)((total, nBins) => total + statsSize * nBins) + } + + /** + * Number of elements for each node, corresponding to stride between nodes in [[_allStats]]. + */ + private val nodeStride: Int = featureOffsets.last + + /** + * Total number of elements stored in this aggregator. + */ + def allStatsSize: Int = numNodes * nodeStride + + /** + * Flat array of elements. + * Index for start of stats for a (node, feature, bin) is: + * index = nodeIndex * nodeStride + featureOffsets(featureIndex) + binIndex * statsSize + * Note: For unordered features, the left child stats precede the right child stats + * in the binIndex order. + */ + protected val _allStats: Array[Double] = new Array[Double](allStatsSize) + + /** + * Get flat array of elements stored in this aggregator. + */ + protected def allStats: Array[Double] = _allStats + + /** + * Update the stats for a given (node, feature, bin) for ordered features, using the given label. + */ + def update( + nodeIndex: Int, + featureIndex: Int, + binIndex: Int, + label: Double, + instanceWeight: Double): Unit = { + val i = nodeIndex * nodeStride + featureOffsets(featureIndex) + binIndex * statsSize + impurityAggregator.update(_allStats, i, label, instanceWeight) --- End diff -- In `DTStatsAggregatorFixedFeatures` and `DTStatsAggregatorSubsampledFeatures`, `impurityAggregator.update` is called with `_allStats`, and in `DTStatsAggregator` it is called with `allStats`. I think we make it consistent by making a function `updateAggregator` in `DTStatsAggregator` like this: ``` def updateAggregator(offset: Int, label: Double, instanceWeight: Double): Unit = { impurityAggregator.update(allStats, offset, label, instanceWeight) } ``` all other functions that need to call `impurityAggragator.update` call this function.
--- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org