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

Fix an issue with PCA not working with Float32 arrays #11

Merged
merged 1 commit into from Oct 12, 2015

Conversation

maximsch2
Copy link
Contributor

Call to fit(PCA, X) fails if X is Float32 array. This pull request adds a test and fixes it. I'm also fixing pcacov and pcasvd as they seem to have the same issue.

@maximsch2
Copy link
Contributor Author

Those build failures seem to be due to issues in JSON.jl and not the PR.

@@ -74,7 +74,7 @@ end

const default_pca_pratio = 0.99

function check_pcaparams{T<:FloatingPoint}(d::Int, mean::Vector{T}, md::Int, pr::T)
function check_pcaparams{T<:FloatingPoint}(d::Int, mean::Vector{T}, md::Int, pr)
Copy link
Member

Choose a reason for hiding this comment

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

pr should still retain FloatingPoint type declaration (here and in the functions below)

@maximsch2
Copy link
Contributor Author

Thanks for the comments! I've added more tests, removed convert and put FloatingPoint there.

@alyst
Copy link
Member

alyst commented Jul 14, 2015

@maximsch2 Thanks. Could you please also update the declaration of pratio in `pcacov()and then squash your commits into a single one? The bigger remaining issue is that now the nightly Julia branch fails inpcacov()`` in one of your newly added tests. Have you looked into that?

@maximsch2
Copy link
Contributor Author

Fixed & squashed. I've tried reproducing the error that travis gets, but I don't get it here.

@maximsch2
Copy link
Contributor Author

Added a fix for deprecated FloatingPoint in pca.jl as well. Tests pass locally for me, on 0.3 on Travis and 0.5-dev on Travis seems to be broken in an unrelated way.
@alyst Can you take a look at it?

@jiahao
Copy link
Member

jiahao commented Oct 8, 2015

I believe the test failure on the nightlies was caused by JuliaLang/julia#13465

/cc @andreasnoack please have a look

@andreasnoack
Copy link
Member

Wow. That is a lot of warnings, but I found the error, and it is indeed because of JuliaLang/julia#13465. I'll have to take another look at the deprecation logic.

@andreasnoack
Copy link
Member

I believe JuliaLang/julia#13505 should fix this issue

andreasnoack added a commit that referenced this pull request Oct 11, 2015
andreasnoack added a commit that referenced this pull request Oct 11, 2015
which is handled in #11. Also enable testing on 0.3 and 0.4.
andreasnoack added a commit that referenced this pull request Oct 11, 2015
which is handled in #11. Also enable testing on 0.3 and 0.4.
@andreasnoack
Copy link
Member

@maximsch2 Would you try to rebase your branch on top of master and push force to your fork. I've made some changes to the .travis.yml file that might fix the test here. Thanks.

@maximsch2
Copy link
Contributor Author

Hm, now the test fails on 0.4 on Linux (it works on OS X on travis as well as on Ubuntu 14.04 locally here). It looks like that's probably just a rounding error. Do we really need that check?

@andreasnoack
Copy link
Member

Maybe the check will catch a blunder some day, but I don't this it is that important. You could also fix the test error by limiting the number of principal components to d-1 or by setting tval=max(vsum,sum(v)). I think you can just choose the one you prefer.

@maximsch2
Copy link
Contributor Author

Added maxoutdim=3 to the test (nearby tests seem to do the same). Now Travis is happy.

andreasnoack added a commit that referenced this pull request Oct 12, 2015
Fix an issue with PCA not working with Float32 arrays
@andreasnoack andreasnoack merged commit 39cba8d into JuliaStats:master Oct 12, 2015
@andreasnoack
Copy link
Member

Thanks.

@maximsch2
Copy link
Contributor Author

Awesome, thanks!

@maximsch2 maximsch2 deleted the pull-request/9606ae58 branch October 12, 2015 04:08
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.

None yet

4 participants