Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/14597#discussion_r75451038
  
    --- 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 --
    
    I agree with your second paragraph. I don't think it's very common that 
someone would perform different chi-squared selections on the same values but 
with different criteria. Therefore I'm not concerned about recomputing 
different models for different criteria. So I think indeed the model does not 
need parameters, and then it's consistent. Does that match your idea?


---
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]

Reply via email to