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 Cdist - Preserve dimensions when casting a vector to double #99

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mardukbp
Copy link
Contributor

Using FastR release 19.2.0.1 the following code

m <- matrix(c(1L,2L,3L,4L,5L,6L), nrow=3, ncol=2)
dist(m)

results in Error: 'x' must be a square matrix.

The R function dist calls the C function Cdist defined in stats/src/distance.c. This function does not check whether its first argument is a matrix, but it checks the type of its elements:

if(TYPEOF(x) != REALSXP) x = coerceVector(x, REALSXP);

FastR implements the Java function Cdist, which checks whether its first argument is a matrix--not a square matrix--and if the test fails it throws

error(Message.MUST_BE_SQUARE_MATRIX, "x")

Throwing this error is wrong, because it does not correspond to the test that failed and because R's Cdist works for rectangular matrices. The Message enum is defined in RError.java.

The problem is that the argument x is casted to double without preserving its dimensions, resulting in a 1D vector. This patch corrects this.

To try it make clean in com.oracle.truffle.r.native and run mx buildon the project root.

I suspect that this mistake in casting vectors to double without preserving their dimensions occurs in other places in FastR.

@steve-s
Copy link
Member

steve-s commented Sep 17, 2019

Hello Marduk,

thank you for your PR and thorough description. The PR looks good but could you also add a test with the code snippet that was not working before (or more misbehaving snippets if you have more)? The tests are located here: https://github.com/oracle/fastr/blob/master/com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/library/base/TestSimpleArithmetic.java. edit: I meant com.oracle.truffle.r.test/src/com/oracle/truffle/r/test/library/stats and looking at that package, it seems we have no tests for Cdist yet..

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

Successfully merging this pull request may close these issues.

3 participants