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 support for dtCMatrix&dsCMatrix #135

Merged
merged 8 commits into from Jun 18, 2017

Conversation

Projects
None yet
4 participants
@binxiangni
Contributor

binxiangni commented Jun 14, 2017

Now I just add codes for the conversion of dtCMatrix and dsCMatrix here in order to partially solve the issue#17 and issue#114. More conversion of sparse matrix would be added soon. Please do me a favor to check the code style or anything else important.

// Setting the sentinel
arma::access::rw(res.col_ptrs[(unsigned) dims[1] + 1]) =
arma::access::rw(res.col_ptrs[static_cast<unsigned>(ncol + 1)]) =

This comment has been minimized.

@coatless

coatless Jun 14, 2017

Contributor

+1 for the static_cast usage.

@coatless

coatless Jun 14, 2017

Contributor

+1 for the static_cast usage.

@coatless

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 14, 2017

Member

Nice! And don't worry too about indentation -- following what is in the other files in the repo is a good plan.

Member

eddelbuettel commented Jun 14, 2017

Nice! And don't worry too about indentation -- following what is in the other files in the repo is a good plan.

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jun 15, 2017

Contributor

I am curious about the meaning of the first dot and triple colons in the following codes:

    .setUp <- RcppArmadillo:::unit_test_setup("sparse.cpp") 
Contributor

binxiangni commented Jun 15, 2017

I am curious about the meaning of the first dot and triple colons in the following codes:

    .setUp <- RcppArmadillo:::unit_test_setup("sparse.cpp") 
@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Jun 15, 2017

Contributor

Generally, the . preceding a function or variable name is meant to hide it from users in a way that is similar to how dotfiles are hidden on most file systems. Basically, these functions and variables are to be considered internal components of the package that shouldn't be modified.

Regarding the :::, this relates to package namespaces. When you call library(pkgname), you are loading all of the packages exported functions and variables into the search path. If you do not, one way to access a package's function is to prefix it with the package name and two colons, e.g. pkgname::function(). Now, you might have components that are not exported because they are internal components as mentioned above. To access these internal values, you can add one additional colon after the package name but before a function or variable, e.g. RcppArmadillo:::unit_test_setup, and viola you have access to it outside the package.

You might want to glance at Double Colon and Triple Colon Operators help doc article for more.

Contributor

coatless commented Jun 15, 2017

Generally, the . preceding a function or variable name is meant to hide it from users in a way that is similar to how dotfiles are hidden on most file systems. Basically, these functions and variables are to be considered internal components of the package that shouldn't be modified.

Regarding the :::, this relates to package namespaces. When you call library(pkgname), you are loading all of the packages exported functions and variables into the search path. If you do not, one way to access a package's function is to prefix it with the package name and two colons, e.g. pkgname::function(). Now, you might have components that are not exported because they are internal components as mentioned above. To access these internal values, you can add one additional colon after the package name but before a function or variable, e.g. RcppArmadillo:::unit_test_setup, and viola you have access to it outside the package.

You might want to glance at Double Colon and Triple Colon Operators help doc article for more.

@@ -77,7 +77,9 @@ namespace traits {
private:
MATRIX mat ;
};
// 14 June 2017

This comment has been minimized.

@thirdwing

thirdwing Jun 15, 2017

Member

I think we don't have time in other place, right?

@thirdwing

thirdwing Jun 15, 2017

Member

I think we don't have time in other place, right?

This comment has been minimized.

@binxiangni

binxiangni Jun 15, 2017

Contributor

I just followed the style of the source code. There are also dates there

@binxiangni

binxiangni Jun 15, 2017

Contributor

I just followed the style of the source code. There are also dates there

This comment has been minimized.

@thirdwing

thirdwing Jun 15, 2017

Member

It doesn't matter. Let's focus on the unit test.

@thirdwing

thirdwing Jun 15, 2017

Member

It doesn't matter. Let's focus on the unit test.

std::copy(p.begin(), p.end(), arma::access::rwp(res.col_ptrs));
std::copy(x.begin(), x.end(), arma::access::rwp(res.values));
// Get the type of sparse matrix
std::string type = Rcpp::as<std::string>(mat.slot("class"));

This comment has been minimized.

@thirdwing

thirdwing Jun 15, 2017

Member

Maybe we can print some message when converting the matrix type?

Like "Converting dgCMatrix into Armadillo type".

What is your opinion? @coatless

@thirdwing

thirdwing Jun 15, 2017

Member

Maybe we can print some message when converting the matrix type?

Like "Converting dgCMatrix into Armadillo type".

What is your opinion? @coatless

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 15, 2017

Member

IIRC the sample() code by Christian has dates in it. I am with you that it is somewhat redundant given the commit logs. But hey, documentation is good.

As for printing message: Hm. Maybe if and only if a verbose flag or option is set?

Member

eddelbuettel commented Jun 15, 2017

IIRC the sample() code by Christian has dates in it. I am with you that it is somewhat redundant given the commit logs. But hey, documentation is good.

As for printing message: Hm. Maybe if and only if a verbose flag or option is set?

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Jun 15, 2017

Contributor

Perhaps a set of debug macros similar to what Rcpp does for some of the String and general conversions?

c.f.

https://github.com/RcppCore/Rcpp/blob/8f2a101789b29e572bf3abe5115d9ddb48c1b0c7/inst/include/Rcpp/String.h#L30-L41

https://github.com/RcppCore/Rcpp/blob/6f81b4684481dbd9bb554dd95e66725fc3b63a8c/inst/include/Rcpp/vector/converter.h#L50

So, we would have

#define DEBUG_RCPPARMADILLO_SPARSE

Then something print status of conversion to console with Rprintf() instead of Rcpp::Rcout?

Contributor

coatless commented Jun 15, 2017

Perhaps a set of debug macros similar to what Rcpp does for some of the String and general conversions?

c.f.

https://github.com/RcppCore/Rcpp/blob/8f2a101789b29e572bf3abe5115d9ddb48c1b0c7/inst/include/Rcpp/String.h#L30-L41

https://github.com/RcppCore/Rcpp/blob/6f81b4684481dbd9bb554dd95e66725fc3b63a8c/inst/include/Rcpp/vector/converter.h#L50

So, we would have

#define DEBUG_RCPPARMADILLO_SPARSE

Then something print status of conversion to console with Rprintf() instead of Rcpp::Rcout?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 15, 2017

Member

Let's not get distracted. This is Binxiang's issue ticket on something different.

We can always print some debug text during development and clean up before releases. No biggie either way.

Member

eddelbuettel commented Jun 15, 2017

Let's not get distracted. This is Binxiang's issue ticket on something different.

We can always print some debug text during development and clean up before releases. No biggie either way.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 16, 2017

Member

@binxiangni The pull request looks really good. Two more suggestions / requests

  1. Tests would be excellent, and if you look at the existing directory it should become clear how to add them. If it is not, well then we can merge this as is and open a new branch just for tests. They are important and a key part of how we do things.

  2. ChangeLog entries would be nice. That gets autoformatted if you use Emacs, it is pretty easy to do by hand as well. Date, name and email with two spaces, then a bullet point for each changed file with a summary.

Can we add that?

Member

eddelbuettel commented Jun 16, 2017

@binxiangni The pull request looks really good. Two more suggestions / requests

  1. Tests would be excellent, and if you look at the existing directory it should become clear how to add them. If it is not, well then we can merge this as is and open a new branch just for tests. They are important and a key part of how we do things.

  2. ChangeLog entries would be nice. That gets autoformatted if you use Emacs, it is pretty easy to do by hand as well. Date, name and email with two spaces, then a bullet point for each changed file with a summary.

Can we add that?

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jun 16, 2017

Contributor

@eddelbuettel Sorry for delay. I'll do that ASAP. Please do not merge for now. I understand the importance of test.

As I mentioned before, my broken laptop quite let me down. Yesterday I tried to use Ubuntu Server in the digitalocean as KK suggests and set up a GUI for further working environment, but GUI didn't work. I'll go to a local dealer to get a new laptop today. After that, I can go back to the right track. Apologize again!

Contributor

binxiangni commented Jun 16, 2017

@eddelbuettel Sorry for delay. I'll do that ASAP. Please do not merge for now. I understand the importance of test.

As I mentioned before, my broken laptop quite let me down. Yesterday I tried to use Ubuntu Server in the digitalocean as KK suggests and set up a GUI for further working environment, but GUI didn't work. I'll go to a local dealer to get a new laptop today. After that, I can go back to the right track. Apologize again!

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 16, 2017

Member

No worry, you are doing great. We could do it as two PRs as well but may as well have you expand this one.

Member

eddelbuettel commented Jun 16, 2017

No worry, you are doing great. We could do it as two PRs as well but may as well have you expand this one.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 16, 2017

Member

Oh, and sorry about the hardware troubles.

@coatless Any way a student can access a proper and current dev env on campus? Isn't that what the VM pool is for? But not outside of classes?

Member

eddelbuettel commented Jun 16, 2017

Oh, and sorry about the hardware troubles.

@coatless Any way a student can access a proper and current dev env on campus? Isn't that what the VM pool is for? But not outside of classes?

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jun 16, 2017

Contributor

The local dealer also tells me that I can get the laptop after 10 days. 😭
Now I try to install Rstudio in the ubuntu server in the digitalocean. There is a step not making sense to me

RStudio can be access through port 8787.

Do you have any idea?

Contributor

binxiangni commented Jun 16, 2017

The local dealer also tells me that I can get the laptop after 10 days. 😭
Now I try to install Rstudio in the ubuntu server in the digitalocean. There is a step not making sense to me

RStudio can be access through port 8787.

Do you have any idea?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 16, 2017

Member

Sure. Standard RStudio Server use via a webbrowser. See eg https://support.rstudio.com/hc/en-us/articles/200552306-Accessing-RStudio-Server-Open-Source

Really no resources on campus?

Member

eddelbuettel commented Jun 16, 2017

Sure. Standard RStudio Server use via a webbrowser. See eg https://support.rstudio.com/hc/en-us/articles/200552306-Accessing-RStudio-Server-Open-Source

Really no resources on campus?

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jun 16, 2017

Contributor

I can access the computer in the computer lab, but have no admin right to install package like Armadillo. But for now, I have an old laptop from my friend and at least it can run the build check in the RStudio. Previously, I think I have to install C++ Armadillo library. Maybe I am wrong. Only if the build check runs and no error occurs, that means it passes the test, right?

Contributor

binxiangni commented Jun 16, 2017

I can access the computer in the computer lab, but have no admin right to install package like Armadillo. But for now, I have an old laptop from my friend and at least it can run the build check in the RStudio. Previously, I think I have to install C++ Armadillo library. Maybe I am wrong. Only if the build check runs and no error occurs, that means it passes the test, right?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 16, 2017

Member

Also, you could just use GitHub and Travis CI. If you clone RcppArmadillo you have a working .travis.yml. So you just need to turn on Travis CI for your account as everything else is already there. (For what it is worth, the first Google hit was this tutorial.)

Member

eddelbuettel commented Jun 16, 2017

Also, you could just use GitHub and Travis CI. If you clone RcppArmadillo you have a working .travis.yml. So you just need to turn on Travis CI for your account as everything else is already there. (For what it is worth, the first Google hit was this tutorial.)

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Jun 16, 2017

Contributor

Just to give a heads up, I've put in a request to get @binxiangni access to a linux machine on campus. I'll let you know what happens. (It likely won't be until Monday since it's almost 5 o'clock.)

Contributor

coatless commented Jun 16, 2017

Just to give a heads up, I've put in a request to get @binxiangni access to a linux machine on campus. I'll let you know what happens. (It likely won't be until Monday since it's almost 5 o'clock.)

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 16, 2017

Member

Thanks for adding tests. Looks like one failed ... but I am about to cook dinner so can't check now.

I am sure we'll get it sorted out.

Member

eddelbuettel commented Jun 16, 2017

Thanks for adding tests. Looks like one failed ... but I am about to cook dinner so can't check now.

I am sure we'll get it sorted out.

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jun 17, 2017

Contributor

I find where the bug is:
'S4' was not declared in this scope
I just fixed apparent bugs but don't know how to include S4 in the scope.

Contributor

binxiangni commented Jun 17, 2017

I find where the bug is:
'S4' was not declared in this scope
I just fixed apparent bugs but don't know how to include S4 in the scope.

@binxiangni

This comment has been minimized.

Show comment
Hide comment
@binxiangni

binxiangni Jun 17, 2017

Contributor

I did unit test locally and it shows R CMD check succeeded, which I guess means that the unit tests pass. But there is a warning: Problems with news in 'inst/NEWS.Rd':. Anyone can give me a hint?

Contributor

binxiangni commented Jun 17, 2017

I did unit test locally and it shows R CMD check succeeded, which I guess means that the unit tests pass. But there is a warning: Problems with news in 'inst/NEWS.Rd':. Anyone can give me a hint?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 17, 2017

Member

Problems with news in 'inst/NEWS.Rd'

That usually means a closing } is missing or in the wrong. And as you didn't touch the file, not your problem :)

I can take a look in a bit. Just in a from a long run...

Member

eddelbuettel commented Jun 17, 2017

Problems with news in 'inst/NEWS.Rd'

That usually means a closing } is missing or in the wrong. And as you didn't touch the file, not your problem :)

I can take a look in a bit. Just in a from a long run...

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Jun 17, 2017

Member

Passes with flying colors for me too!

Member

eddelbuettel commented Jun 17, 2017

Passes with flying colors for me too!

@eddelbuettel eddelbuettel merged commit ab72955 into RcppCore:master Jun 18, 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