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

Rewrite conversion for dgT, dtT & dsTMatrix & add file-based tests #142

Merged
merged 1 commit into from Jul 6, 2017

Conversation

Projects
None yet
3 participants
@binxiangni
Contributor

binxiangni commented Jul 5, 2017

First, the file-based tests are based on the documentation from package Matrix, sparseM, spam, slam, SciPy.

Second, when I did the file-based tests, I found two extreme cases that the original conversion for TsparseMatrix cannot satisfy, so I rewrite the conversion.

The two new cases include:

  1. i is increasingly ordered;
  2. (i, j) pair is repeated.

Last, I've passed the tests, but there is a warning that
Warning in read_symbols_from_dll(so, rarch) :
this requires 'objdump.exe' to be on the PATH
Should I do something?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 5, 2017

Member

The change to the header file looks good and careful -- though I only glanced at it. It is quite possible that you fixed an error which had been hiding there for some time.

I am wondering if we can do better on the (short) included cpp file? Ie for the rest of the package we have the cpp files in this directory which already has a file for sparse matrix tests. Could you use that mechanism, and call via unit test functions as in this file ?

Member

eddelbuettel commented Jul 5, 2017

The change to the header file looks good and careful -- though I only glanced at it. It is quite possible that you fixed an error which had been hiding there for some time.

I am wondering if we can do better on the (short) included cpp file? Ie for the rest of the package we have the cpp files in this directory which already has a file for sparse matrix tests. Could you use that mechanism, and call via unit test functions as in this file ?

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jul 5, 2017

Contributor

Maybe I don't totally understand what you mean, but for now,I just call the cpp file to do the unit tests. The unit test is so long just because I have to write a new triangular and a new symmetric matrix to test them.

Contributor

binxiangni commented Jul 5, 2017

Maybe I don't totally understand what you mean, but for now,I just call the cpp file to do the unit tests. The unit test is so long just because I have to write a new triangular and a new symmetric matrix to test them.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 5, 2017

Member

You added two files to subdirectory tests/. That is useful for the simplest test cases. But yours is already more complex.

And what the package does internally is to use the RUnit package to have the file tests/doRUnit.R automatically call everything in the unit tests directory.

And we already have 1) a file for sparse tests there which you could add two, or complement witha new one, and 2) a C++ file there to support it -- so you short C++ function could go there.

Have a look at those files, and what happens when you do R CMD check RcppArmadillo_*tar.gz. It would be better to have your new tests there.

Member

eddelbuettel commented Jul 5, 2017

You added two files to subdirectory tests/. That is useful for the simplest test cases. But yours is already more complex.

And what the package does internally is to use the RUnit package to have the file tests/doRUnit.R automatically call everything in the unit tests directory.

And we already have 1) a file for sparse tests there which you could add two, or complement witha new one, and 2) a C++ file there to support it -- so you short C++ function could go there.

Have a look at those files, and what happens when you do R CMD check RcppArmadillo_*tar.gz. It would be better to have your new tests there.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 5, 2017

Member

Also, what we could do is to

  • first merge as is
  • then post-process to convert into the standard format

I could do that if you want me to. And I'll make sure you end up in the corresponding copyright headers.

Member

eddelbuettel commented Jul 5, 2017

Also, what we could do is to

  • first merge as is
  • then post-process to convert into the standard format

I could do that if you want me to. And I'll make sure you end up in the corresponding copyright headers.

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jul 5, 2017

Contributor

I've integrated the previous files into the unitTests folder. Please help me check if the format and copyright headers are appropriate.

And I passed the tests locally, but it's weird it didn't work on the website. I checked the log and the error happened runit.sparse.R. But I didn't touch it.

Contributor

binxiangni commented Jul 5, 2017

I've integrated the previous files into the unitTests folder. Please help me check if the format and copyright headers are appropriate.

And I passed the tests locally, but it's weird it didn't work on the website. I checked the log and the error happened runit.sparse.R. But I didn't touch it.

@thirdwing

This comment has been minimized.

Show comment
Hide comment
@thirdwing

thirdwing Jul 5, 2017

Member

It seems the unit test also passed on my machine.

Let's restart the Travis and see what happens.

Member

thirdwing commented Jul 5, 2017

It seems the unit test also passed on my machine.

Let's restart the Travis and see what happens.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 5, 2017

Member

Thanks for making the chamges! I can take a look tomorrow. Bed-time here in Brussels...

Member

eddelbuettel commented Jul 5, 2017

Thanks for making the chamges! I can take a look tomorrow. Bed-time here in Brussels...

@thirdwing

This comment has been minimized.

Show comment
Hide comment
@thirdwing

thirdwing Jul 5, 2017

Member

@binxiangni I am afraid you are using a wrong format for RUnit.

For example, in https://github.com/binxiangni/RcppArmadillo/blob/master/inst/unitTests/runit.sparseConversion.R#L39-L43

set.seed(7)
m <- matrix(0, 5, 5)
m[sample(length(m), size = 14)] <- rep(1:9, length=14)
mm <- as(m, "CsparseMatrix")
checkEquals(mm, asSpMat(mm), msg="dgC2dgC")

This should be

    test.as.dgC2dgC <- function() {
        set.seed(7)
        m <- matrix(0, 5, 5)
        m[sample(length(m), size = 14)] <- rep(1:9, length=14)
        mm <- as(m, "CsparseMatrix")
        checkEquals(mm, asSpMat(mm), msg="dgC2dgC")
    }

This should make Travis CI happy. See https://travis-ci.org/thirdwing/RcppArmadillo

Member

thirdwing commented Jul 5, 2017

@binxiangni I am afraid you are using a wrong format for RUnit.

For example, in https://github.com/binxiangni/RcppArmadillo/blob/master/inst/unitTests/runit.sparseConversion.R#L39-L43

set.seed(7)
m <- matrix(0, 5, 5)
m[sample(length(m), size = 14)] <- rep(1:9, length=14)
mm <- as(m, "CsparseMatrix")
checkEquals(mm, asSpMat(mm), msg="dgC2dgC")

This should be

    test.as.dgC2dgC <- function() {
        set.seed(7)
        m <- matrix(0, 5, 5)
        m[sample(length(m), size = 14)] <- rep(1:9, length=14)
        mm <- as(m, "CsparseMatrix")
        checkEquals(mm, asSpMat(mm), msg="dgC2dgC")
    }

This should make Travis CI happy. See https://travis-ci.org/thirdwing/RcppArmadillo

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jul 6, 2017

Contributor

Now the new unit test file should be fine. This is a good lesson for me to learn how to do unit tests in RUnit. Thanks for instruction. :)

Contributor

binxiangni commented Jul 6, 2017

Now the new unit test file should be fine. This is a good lesson for me to learn how to do unit tests in RUnit. Thanks for instruction. :)

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jul 6, 2017

Member

Yes, looks much better :)

With this in line 552:

# ....(To be continued)

Do you want to complete this, or shall we merge now?

Member

eddelbuettel commented Jul 6, 2017

Yes, looks much better :)

With this in line 552:

# ....(To be continued)

Do you want to complete this, or shall we merge now?

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jul 6, 2017

Contributor

That's for the next stage, since some of conversion is not achieved now. I just note that in order to remind myself. You can merge that, if no error is found.

Contributor

binxiangni commented Jul 6, 2017

That's for the next stage, since some of conversion is not achieved now. I just note that in order to remind myself. You can merge that, if no error is found.

@eddelbuettel eddelbuettel merged commit e682128 into RcppCore:master Jul 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment