markmfredrickson / optmatch

Functions for optimal matching in R
https://markmfredrickson.github.io/optmatch
Other
47 stars 14 forks source link

`subset.InfinitySparseMatrix` problem; implicit integer class of row & col slots #93

Closed benthestatistician closed 9 years ago

benthestatistician commented 9 years ago

Per the definition of the InfinitySparseMatrix class, row and col are numerics. However, the C function subsetInfSparseMatrixseems to expect integers here. This causessubset.ISM` to fail, as in this example:

A <- optmatch:::makeInfinitySparseMatrix(c(1,2,3), cols = c(1,2, 2), rows = c(1,1,2))
subset(A, rep(T,2), rep(T,2)) # quits & complains
A@cols <- as.integer(A@cols)
A@rows <- as.integer(A@rows)
subset(A, rep(T,2), rep(T,2)) # happy now

Here subset.ISM fails because of the problem. It can also create the problem, by spitting out ISMs w/ non-integer rows and cols, even when the ISM that went into it had integer rows and cols.

At a minimum, makeISM and subset.ISM should be outfitted to return objects with integer rows and cols slots. I'd also suggest that the ISM class definition should make the integer requirement explicit. Assigning to Mark for comment, in case he's aware of likely side effects that I'm unaware of.

nullsatz commented 9 years ago

This is my bad. When I wrote the C code, I just assumed that I would be getting handed integers.

I have liberally added as.integer around and updated a tiny section of roxygen docu.

The changes are in the issue93 branch from master and tests clean.

benthestatistician commented 9 years ago

Looks good to me. Mark, could you do a quick review and then pass-back to Josh B for changes, merging back into master and closing the issue, as appropriate?

markmfredrickson commented 9 years ago

Looks good. Merged into master.