Skip to content

Commit

Permalink
fixes ndim = 1 bugs, closes #37 #38 #39
Browse files Browse the repository at this point in the history
  • Loading branch information
@gdkrmr
gdkrmr committed Jan 28, 2019
1 parent de1e161 commit fbd7bbf
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 2 deletions.
3 changes: 2 additions & 1 deletion R/diffmap.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ DiffusionMaps <- setClass(
maxdim = pars$ndim,
delta = pars$delta
)
outdata <- diffres$X
outdata <- as.matrix(diffres$X)


appl <- function(x) {
appl.meta <- if (inherits(x, "dimRedData")) x@meta else data.frame()
Expand Down
2 changes: 2 additions & 0 deletions R/hlle.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ HLLE <- setClass(
chckpkg("Matrix")
chckpkg("RANN")

if (pars$ndim < 2) stop("ndim must be 2 or larger.")

if (is.null(pars$knn)) pars$knn <- 50
if (is.null(pars$ndim)) pars$ndim <- 2

Expand Down
2 changes: 1 addition & 1 deletion R/l1pca.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ PCA_L1 <- setClass(
## evaluate results here for functions
data <- res$scores
colnames(data) <- paste0("PC", seq_len(ndim))
rot <- res$loadings[, seq_len(ndim)]
rot <- as.matrix(res$loadings[, seq_len(ndim)])
dimnames(rot) <- list(orgnames, newnames)
rerot <- t(rot)

Expand Down
6 changes: 6 additions & 0 deletions tests/testthat/test_HLLE.R
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
context("HLLE")

test_that("HLLE", {
expect_error(embed(iris[1:4], "HLLE", ndim = 1, .mute = c("message", "output")),
"ndim must be 2 or larger.")
})
3 changes: 3 additions & 0 deletions tests/testthat/test_PCA_L1.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,4 +78,7 @@ test_that("general data conversions", {
getData( getDimRedData(irisRes) )
)

expect_s4_class({ embed(iris[1:4], "PCA_L1", ndim = 1,
.mute = c("message", "output")) },
"dimRedResult")
})
10 changes: 10 additions & 0 deletions tests/testthat/test_diffmap.R
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
context("DiffusionMaps")

test_that("DiffusionMaps", {
expect_s4_class(embed(iris[1:4], "DiffusionMaps", ndim = 1,
.mute = c("message", "output")),
"dimRedResult")
x <- embed(iris[1:4], "DiffusionMaps", ndim = 1,
.mute = c("message", "output"))
expect_equal(dim(x@data@data), c(150, 1))
})

3 comments on commit fbd7bbf

@khughitt
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fixes!

Couple quick side notes:

  1. You may have already been bitten by this in R, but it's sometimes helpful to include a drop=FALSE arg when sub-setting matrices or dataframe columns in cases where it is possible that only one column will be selected. This ensures that an m x 1 matrix/dataframe is returned instead of a vector. E.g. compare iris[, 4] and iris[, 4, drop = FALSE].

  2. Do you have any desire / plans to extend dimRed to include other methods in the future? (Other PCA/CCA variants, UMAP, tensor decomp methods, etc. come to mind..) If so, what kind of scope do you have in mind? Perhaps I could start a list somewhere of other methods that are already implemented in R that could be interested to eventually include, and If I have some time down the road, I can try and implement some of them.

@gdkrmr
Copy link
Owner Author

@gdkrmr gdkrmr commented on fbd7bbf Jan 28, 2019

Choose a reason for hiding this comment

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

To 1) yes I already was and I (and the creators of R) think that this is a terrible default. Thanks for reminding me.
To 2) I currently have a fixed interface that maps a matrix (obs x dims) to a matrix (obs x embedding dims).

  • More PCA variants will be simple, which ones where you thinking about?
  • CCA is a little bit more complicated, but doable, if you are only interested in one of the projection matrices.
  • UMAP is already implemented (https://github.com/gdkrmr/dimRed/blob/master/R/umap.R).
  • Tensor decomposition is very different and I don't think it will fit the interface without a large overhaul.

If you find more methods to wrap or implement, feel free to propose them in #4. Currently I don't have a lot of time to dedicate to this, so I don't want to make to many promises.

PRs are always welcome!

@khughitt
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! The two PCA variants that come to mind are sparse and robust PCA. I'll add them to #4, and if I have the time and it seems useful, perhaps I'll submit a PR at some point.

Thanks for the quick fixes with the n=1 issues!

Please sign in to comment.