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

add labels to prcomp object before tidying #265

Merged
merged 6 commits into from Jun 6, 2018

Conversation

Projects
None yet
2 participants
@corybrunson
Contributor

corybrunson commented Nov 30, 2017

This would fix #254 (see the discussion there for examples).

@alexpghayes

This comment has been minimized.

Collaborator

alexpghayes commented Jun 6, 2018

If you pull in the most recent master the (AppVeyor) build should pass. It'd be nice to add a regression test and then I'd be happy to merge.

Merge branch 'master' into prcomp-labels
Conflicts:
	R/prcomp_tidiers.R
@corybrunson

This comment has been minimized.

Contributor

corybrunson commented Jun 6, 2018

Thanks! I'm having some issues with the build (on a testing branch), but i'll push the merge and see what happens.

@alexpghayes

This comment has been minimized.

Collaborator

alexpghayes commented Jun 6, 2018

Looks good. Would you add a quick test to make sure that the reprex from before:

library(broom)
test <- as.data.frame(matrix(1:9, ncol = 3) + rnorm(n = 9, sd = 0.25))
test
#>          V1       V2       V3
#> 1 0.7707063 3.870905 7.091106
#> 2 2.3385348 4.987070 7.835155
#> 3 2.6638938 6.006691 8.690749
pca <- prcomp(test)
tidy(pca, matrix = "u")
#> Error in data.frame(label = rep(labels, times = ncomp), samples): arguments imply differing number of rows: 0, 9
test2 

doesn't give an error?

@corybrunson

This comment has been minimized.

Contributor

corybrunson commented Jun 6, 2018

It works for me!

@corybrunson

This comment has been minimized.

Contributor

corybrunson commented Jun 6, 2018

Or, did you mean add a test to some part of the PR?

@alexpghayes

This comment has been minimized.

Collaborator

alexpghayes commented Jun 6, 2018

Yeah I meant add a test to the PR so that if the implementation gets changed in the future the error doesn't return.

@alexpghayes

This comment has been minimized.

Collaborator

alexpghayes commented Jun 6, 2018

The test current checks that the original reprex fails. We should instead make sure that it passes. I think you want expect_silent, not expect_error. It's weird that that test passed.

@corybrunson

This comment has been minimized.

Contributor

corybrunson commented Jun 6, 2018

The check uses expect_error() with regexp = NA, indicating that no error should occur, so it should pass. Is there something else wrong?

@alexpghayes

This comment has been minimized.

Collaborator

alexpghayes commented Jun 6, 2018

Woops, my bad. That's it then. Feel free to add yourself as a contributor in DESCRIPTION!

@alexpghayes alexpghayes merged commit df8b61d into tidymodels:master Jun 6, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@corybrunson corybrunson deleted the corybrunson:prcomp-labels branch Jun 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment