Skip to content

Conversation

@dbtsai
Copy link
Member

@dbtsai dbtsai commented Dec 18, 2014

The original test doesn't make sense since if you step in, the lossSum is already NaN,
and the coefficients are diverging. That's because the step size is too large for SGD,
so it doesn't work.

The correct behavior is that you should get smaller coefficients than the one
without regularization. Comparing the values using 20000.0 relative error doesn't
make sense as well.

@SparkQA
Copy link

SparkQA commented Dec 18, 2014

Test build #24595 has started for PR 3735 at commit b1a3c42.

  • This patch merges cleanly.

@jkbradley
Copy link
Member

LGTM, pending Jenkins.

This kind of test seems pretty fragile anyways. I guess it provides a check for optimization behavior changing underneath, but at some point, it might be better to switch to better checks for convergence. E.g., we could use a well-conditioned problem and compare the results after a bunch of iterations from different starting points. The epsilon error allowed could be calculated properly based on the convergenceTol + the objective. Is that too much for now? It could be a JIRA instead.

@SparkQA
Copy link

SparkQA commented Dec 18, 2014

Test build #24595 has finished for PR 3735 at commit b1a3c42.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24595/
Test PASSed.

@dbtsai
Copy link
Member Author

dbtsai commented Dec 18, 2014

I agree. The test is not good. I'm thinking we probably can add couple well known dataset like iris or prostate cancer dataset into the test resource, and we can compare the accuracy with R. At Alpine, we also have small R script as comment in the scala test which can reproduce the coefficients. Of course, we should test if this will be converged to the same solution from different initial condition. I found this issue when I refactorize our internal MLOR code to MLLIb, and I will add more tests in MLOR PR.

@mengxr
Copy link
Contributor

mengxr commented Dec 18, 2014

Merged into master. Thanks!

@asfgit asfgit closed this in 59a49db Dec 18, 2014
@dbtsai dbtsai deleted the mlortestfix branch December 19, 2014 20:30
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.

5 participants