Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit tests for algorithms #61

Open
jsjason opened this issue Jul 17, 2015 · 8 comments
Open

Unit tests for algorithms #61

jsjason opened this issue Jul 17, 2015 · 8 comments
Labels

Comments

@jsjason
Copy link
Contributor

jsjason commented Jul 17, 2015

We don't have any unit tests for algorithms. We should add tests so that we can make sure the algorithms are actually performing intended behaviors.

@dongjoon-hyun
Copy link
Contributor

Hi, @jsjason . For PageRank, I can add unit test, but I'm not sure about the dolphin's policy. Is it okay I do the following way?

  • Make a folder for test suite.
    • /src/test/java/edu/snu/reef/dolphin/examples/ml/algorithms/graph
  • Add the following files.
    • package-info.java
    • GraphTestSuite.java
    • PageRankTest.java

@dongjoon-hyun
Copy link
Contributor

I made a PR #80 for further discussion. :)

@jsjason
Copy link
Contributor Author

jsjason commented Aug 22, 2015

The test files should be in the same packages as the classes they are trying to test except under src/test/ and not src/main/. In that sense, the folder structure you proposed is great. I'll add comments on the PR itself.

@dongjoon-hyun
Copy link
Contributor

Hi, @jsjason . If you don't mind, I will add two test suites for logistic/linear regression.
I think integration tests are better than no tests. How do you think about that? We should have them before moving into REEF-0.13-SNAPSHOT aggressively. How do you think about these?

@jsjason
Copy link
Contributor Author

jsjason commented Aug 27, 2015

Yes, in fact I'd be really thankful. We should definitely have tests for each algorithm before upgrading to the snapshot version.

@dongjoon-hyun
Copy link
Contributor

Thanks. I will make a PR, soon.

@dongjoon-hyun
Copy link
Contributor

Hi, @jsjason . Please review PR #91 .

@dongjoon-hyun
Copy link
Contributor

And, I have no idea for clustering (K-means, EM). If you're okay, what about taking care them later after moving to REEF-0.13.

@jsjason jsjason added the BSP label Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants