Github user mpjlu commented on a diff in the pull request:
https://github.com/apache/spark/pull/14597#discussion_r75419052
--- Diff:
mllib/src/main/scala/org/apache/spark/mllib/feature/ChiSqSelector.scala ---
@@ -189,11 +228,35 @@ class ChiSqSelector @Since("1.3.0") (
*/
@Since("1.3.0")
def fit(data: RDD[LabeledPoint]): ChiSqSelectorModel = {
- val indices = Statistics.chiSqTest(data)
- .zipWithIndex.sortBy { case (res, _) => -res.statistic }
- .take(numTopFeatures)
- .map { case (_, indices) => indices }
- .sorted
+ chiSqTestResult = Statistics.chiSqTest(data)
+ selectorType match {
+ case ChiSqSelectorType.KBest => selectKBest(numTopFeatures)
+ case ChiSqSelectorType.Percentile => selectPercentile(percentile)
+ case ChiSqSelectorType.Fpr => selectFpr(alpha)
+ case _ => throw new Exception("Unknown ChiSqSelector Type")
+ }
+ }
+
+ @Since("2.1.0")
+ def selectKBest(value: Int): ChiSqSelectorModel = {
--- End diff --
Hi @srowen , maybe I misunderstood you. I think make the model instance be
parameterized is the same as current solution (make selector instance be
parameterized). This only difference of your suggestion is moving the logic
from selector to model. Both of the solution is to avoid computing
ChiSqTestResult many times when one wanted to perform different chi squared
selection.
If we don't consider to avoid computing ChiSqTestResult many times, the
problem will be very easy, and can be consistent exactly with other methods.
Can we don't consider reuse ChiSqTestResult in this PR. Thanks
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
