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

WIP Fix 0.7/1.0 Incompatiabilities #34

Merged
merged 25 commits into from
Nov 1, 2018

Conversation

fundamental
Copy link
Contributor

I'm pretty sure I used this project back in the 0.4 era or so and I was trying to use it again yesterday. It failed to load on the build of 0.7 I had due to some dependencies. I updated to 1.0 (being hopeful) and it looks like there's been a fair amount of bitrot due to all of the changes in the core Julia libs/API.

While I certainly haven't fixed all the problems, this set of commits should be a stepping stone in getting GassianMixtures to run again. I'm not sure if I'll have the time to finish updating the code, so if anyone is interested feel free to take over where I left off. The commits should be atomic enough that there aren't any surprises.

@davidavdav
Copy link
Owner

Thanks!

Yes, I haven't used this package in a long time myself. If I had to write it again, I'd probably do it differently this time.

There still are some travis failures. DO you think you can fix these?

@fundamental
Copy link
Contributor Author

This PR is now 1 assert away from passing tests (which isn't to say that everything is working quite right, but it is a start) on 0.7. If the prediction assert at test/scikitlearn.jl:29 is disabled, it 'successfully' runs locally.

@cstjean
Copy link
Contributor

cstjean commented Oct 31, 2018

Wow, that @assert should have been a @test.

It would be interesting to see what value sum(predict(gmm, X_train) .== 1) has. Since there's an srand in there, and Julia's RNG has changed since I wrote that code, it's possible that there's some outlier which is misclassified under srand(42). You might try srand(43) 🙂. Or you could @test_broken it, and someone else will look at it some day. ScikitLearn compatibility shouldn't prevent this package from being updated; thank you for your efforts.

This should resolve testing on 0.7 with this testcase being reserved
for later work
@fundamental
Copy link
Contributor Author

Tests 'pass' on 0.7, 1.0, and nightly now. There's plenty of smaller issues to be addressed, but it may be acceptable to merge this at this stage.

I'll make no guarantee of correctness, though it is less broken (on 0.7/1.0) than it was before.

@@ -49,8 +49,7 @@ ScikitLearnBase.predict_log_proba(gmm::GMM, X) = log(gmmposterior(gmm, X)[1])
ScikitLearnBase.predict_proba(gmm::GMM, X) = gmmposterior(gmm, X)[1]
ScikitLearnBase.predict(gmm::GMM, X) =
# This is just `argmax(axis=2)`. It's very verbose in Julia.
ind2sub(size(X),
vec(findmax(ScikitLearnBase.predict_proba(gmm, X), 2)[2]))[2]
CartesianIndices(size(X))[vec(findmax(ScikitLearnBase.predict_proba(gmm, X), dims=2)[2])][2]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that in 0.7/1.0, this line should just be getindex.(argmax(ScikitLearnBase.predict_proba(gmm, X), dims=2), 2). Then the test might work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed and the test works, though the intent isn't entirely clear given my quick read. Half of the return value is 1 and half is 2, and the assert now passes.

Copy link
Contributor

Choose a reason for hiding this comment

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

The training set has 300 points in gaussian #1 and 600 in gaussian #2. They are cleanly separated. After performing clustering, how many points should be in cluster #1? Either 300 or 600, since we don't know which cluster corresponds to which gaussian.

Corrects indexing of scikitlearn and fixes associated test case
src/scikitlearn.jl Outdated Show resolved Hide resolved
@davidavdav davidavdav merged commit 7d84097 into davidavdav:master Nov 1, 2018
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.

3 participants