Skip to content

Conversation

@Lewuathe
Copy link
Contributor

…sion as followup. This is the follow up work of SPARK-10668.

  • Fix miner style issues.
  • Add test case for checking whether solver is selected properly.

@SparkQA
Copy link

SparkQA commented Oct 20, 2015

Test build #43981 has finished for PR 9180 at commit 11cd9c1.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 20, 2015

Test build #43983 has finished for PR 9180 at commit 28427d2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithBigFeature -> WithManyFeatures or WithLargeFeatureSize

@mengxr
Copy link
Contributor

mengxr commented Oct 20, 2015

@Lewuathe The added test took 30 seconds to run, which might be too long. Shall we try to reduce the number of iterations?

[info] - linear regression model with l-bfgs with big feature datasets (29 seconds, 82 milliseconds)

@dbtsai
Copy link
Member

dbtsai commented Oct 20, 2015

Or you can make them sparse by randomly choosing most of the features zeros.

@mengxr
Copy link
Contributor

mengxr commented Oct 20, 2015

+1 on @dbtsai 's suggestion

@SparkQA
Copy link

SparkQA commented Oct 21, 2015

Test build #44067 has finished for PR 9180 at commit 22ba64e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@Lewuathe
Copy link
Contributor Author

- linear regression model with l-bfgs with big feature datasets (14 seconds, 524 milliseconds)

It takes about the half of the initial one. Could review this again? > @dbtsai @mengxr

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line.

@SparkQA
Copy link

SparkQA commented Oct 22, 2015

Test build #44116 has finished for PR 9180 at commit f6b2256.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@Lewuathe
Copy link
Contributor Author

@dbtsai Could you check again please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about consolidate with LinearDataGenerator, and add sparsity = 1.0 as param to control if it's sparse feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I also thought it is good idea. But LinearDataGenerator is used as static object, then we have to pass sparsity as parameter to generateLinearInput. This method seems to be used a lot of suites. It is necessary to change a lot of method reference.
Therefore it might be better to do in separate JIRA. What do you thing about?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's modify the JIRA and do it here. Basically, you can create a LinearDataGenerator with old signature calling new API for compatibility issue.

@SparkQA
Copy link

SparkQA commented Oct 24, 2015

Test build #44279 has finished for PR 9180 at commit 2082d47.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 25, 2015

Test build #44308 has finished for PR 9180 at commit 003d3bd.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 25, 2015

Test build #44313 has finished for PR 9180 at commit 0a43033.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 25, 2015

Test build #44317 has finished for PR 9180 at commit 59383fd.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@Lewuathe
Copy link
Contributor Author

@dbtsai Sorry for bothering many times but could check again please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting in pervious method

  def generateLinearInput(
      intercept: Double,
      weights: Array[Double],
      nPoints: Int,
      seed: Int,
      eps: Double = 0.1): Seq[LabeledPoint] = {
    generateLinearInput(intercept, weights,
      Array.fill[Double](weights.length)(0.0),
      Array.fill[Double](weights.length)(1.0 / 3.0),
      nPoints, seed, eps)}

looks weird for me. Can you fix in this PR? Thanks.

@dbtsai
Copy link
Member

dbtsai commented Oct 29, 2015

Sorry for the delay. The current implementation of creating sparse features is not efficient since we need to create dense feature first. Let's do it as it. But if you are interested in, let's create another JIRA such that the sparse features can be generated without doing dense one. Thanks.

@Lewuathe
Copy link
Contributor Author

@dbtsai Thank you so much for reviewing even you would be busy in Spark Summit. I'll update.

@SparkQA
Copy link

SparkQA commented Oct 30, 2015

Test build #44667 has finished for PR 9180 at commit 97c76c9.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 30, 2015

Test build #44669 has finished for PR 9180 at commit 74de81e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make seed = seed into just seed

@dbtsai
Copy link
Member

dbtsai commented Oct 30, 2015

LGTM except the small styling issues.

@SparkQA
Copy link

SparkQA commented Oct 30, 2015

Test build #44673 has finished for PR 9180 at commit 241ec72.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dbtsai
Copy link
Member

dbtsai commented Oct 30, 2015

Thanks. Merged into master.

@asfgit asfgit closed this in 86d6526 Oct 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants