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

Seamlessly removing the use of backingpath for sub and attach. #56

Merged
merged 7 commits into from
Jan 2, 2017
Merged

Conversation

privefl
Copy link
Contributor

@privefl privefl commented Dec 27, 2016

So,

  • I added a function to get the directory where a FileBackedBigMatrix is stored,
  • I added this info to the description object

so that now, you can use sub.big.matrix and attach.big.matrix without specifying the backingpath parameter (you still can for backward compatibility).

There are lots of man files that have been modified (without real change in the documentation) due to the use of the latest version of roxygen2.

You can see the new test file if it's not clear enough.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 56.214% when pulling 47a6b09 on privefl:master into 65b4430 on kaneplusplus:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.0003%) to 55.999% when pulling 415eb64 on privefl:master into 65b4430 on kaneplusplus:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.1%) to 57.127% when pulling 3b4b002 on privefl:master into 65b4430 on kaneplusplus:master.

@privefl
Copy link
Contributor Author

privefl commented Dec 28, 2016

Wow ok, I've just understood that this PR updates whenever I update my repo.

So, if you want I added a method to efficiently transpose a big.matrix.
Transposing a 10K x 10K big.matrix takes 0.6 sec on my laptop.

@cdeterman
Copy link
Contributor

Hi @privefl, with transpose you are more in my territory with bigalgebra. My fork is the most recent. Please compare yours to the version I have with the armadillo backend. Things like transpose belong in that package.

@privefl
Copy link
Contributor Author

privefl commented Dec 28, 2016

Hi @cdeterman, I've tested matrix multiplication with Armadillo backend and it is very slow (compared for example with Eigen, or directly by blocks within R).
It is why I don't use bigalgebra, because I really care about performances.

Moreover, I'm not sure that transpose is only used for algebra. For example, when parsing some data and storing it in a big.matrix, it may be easier (and even faster) to read the transposed matrix (and transpose it afterwards).

As you (and Mike) prefer.

@cdeterman
Copy link
Contributor

I agree that the default is quite slow but that depends entirely on the BLAS backend being used which by default is the R BLAS which is not very good. If you use OpenBLAS or ATLAS it is much better. This flexibility is the reason armadillo was chose and not Eigen or our own custom scripts.

As for the proper place of transpose this discussion happened before hence why I made the statement here. That said if the others agree it would be better here I wouldn't argue.

@privefl
Copy link
Contributor Author

privefl commented Dec 28, 2016

I'll redo some testing (for matrix multiplication) and open an issue in bigalgebra.

@kaneplusplus kaneplusplus merged commit 384f194 into kaneplusplus:master Jan 2, 2017
@cdeterman
Copy link
Contributor

@kaneplusplus Not to be a stickler but did you want the entire merge here? I am mainly referring to the addition of transpose. I thought we had discussed previously that it should be in bigalgebra? Ultimately I don't care that much but it does matter whether I keep transpose functionality in bigalgebra going forward.

@privefl
Copy link
Contributor Author

privefl commented Jan 3, 2017

If you don't want the transpose feature in this package, just remove the corresponding files in

  • R/
  • src/
  • examples/
  • tests/testthat/
  • man/

They should be independent files.

@kaneplusplus
Copy link
Owner

@cdeterman Thanks for pointing this out. I'll remove now.

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

Successfully merging this pull request may close these issues.

None yet

4 participants