snuspl / dolphin

14 stars 2 forks source link

Unit tests for algorithms #61

Open jsjason opened 9 years ago

jsjason commented 9 years ago

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 commented 9 years ago

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?

dongjoon-hyun commented 9 years ago

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

jsjason commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

dongjoon-hyun commented 9 years ago

Thanks. I will make a PR, soon.

dongjoon-hyun commented 9 years ago

Hi, @jsjason . Please review PR #91 .

dongjoon-hyun commented 9 years ago

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.