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

Eigen speed #42

Merged
merged 13 commits into from
Nov 28, 2018
Merged

Eigen speed #42

merged 13 commits into from
Nov 28, 2018

Conversation

breznak
Copy link
Member

@breznak breznak commented Jul 31, 2018

Small example using Eigen

  • add Eigen support
  • example code optimization

Overall performance might actually be worse, as we need to convert to/from eig. matrix.

Relevant #3
Replaces #14
After #30

@breznak breznak added this to the Optimizations milestone Jul 31, 2018
@breznak breznak self-assigned this Jul 31, 2018
@breznak breznak removed this from the Optimizations milestone Aug 1, 2018
@breznak breznak requested a review from dkeeney November 25, 2018 14:12
@breznak
Copy link
Member Author

breznak commented Nov 25, 2018

@dkeeney please review when you have time, this merely prepares the codebase for using Eigen, with some usage examples in ArrayBase. Should we merge and continue optimizations in other PR, or make it all part of this one?

@chhenning did you mention you use eigen optimizations in your fork? Do you have many of them and what is your experience (speedup, portability, simplicity...?) Can I pull them here?

@ctrl-z-9000-times I figured the ArrayBase class has overlaps with your SDR (#113 ), if you start finding usages and applying SDR to codebase, please have a crack at ArrayBase? :

  • remove what is not (barely) needed (eg copy() routines), and replace with std::
  • get_dense_from_sparse, ... -drop, and use your SDR class instead
  • this class is very old and quite unmaintained, the more is removed, the better.

@breznak breznak added the ready label Nov 25, 2018
@dkeeney
Copy link

dkeeney commented Nov 25, 2018

The ArrayBase (actually the Array class) is the object that passes SDR arrays between Regions (via Links) within the Network framework. Be aware that this is used for non-SDR arrays of various types as well so some thought needs to be put into how we do this. Also note that some Regions are implemented as Python and some in C++ and the Array class passes data between them.

@breznak breznak added this to the optimization milestone Nov 26, 2018
@breznak
Copy link
Member Author

breznak commented Nov 26, 2018

The ArrayBase (actually the Array class) is the object that passes SDR arrays between Regions (via Links) within the Network framework

This may not be the best case for examples. I just had simple example from the past there, so I've implemented that.

lso note that some Regions are implemented as Python and some in C++ and the Array class passes data between them.

This shouldn't be a problem, as Eigen maps the raw C array (T*) memory in O(1). I'll investigate, but seems I'd like to use eigen matrices/vectors as default, same way as numpy is used in python repo.

@breznak
Copy link
Member Author

breznak commented Nov 26, 2018

Should we use same approach as with SDR? Merge this PR introducing the concept, and have usages in separate PR with performance bench?

@breznak
Copy link
Member Author

breznak commented Nov 27, 2018

@dkeeney @ctrl-z-9000-times please review if you have time, I'd like to merge before the big pybind changes

Copy link

@dkeeney dkeeney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that this is several commits behind the master.

I don't know anything about the Eigen algorithm so I cannot comment on whether this is a good thing or not. I will have to take your judgement. I don't see anything that might be a problem. If this needs to be accessed from Python then I will have to figure out how to setup the bindings.

@breznak
Copy link
Member Author

breznak commented Nov 28, 2018

If this needs to be accessed from Python then I will have to figure out how to setup the bindings.

I think Eigen matrices work well with python, we can pass *array.

@breznak breznak merged commit 0354a9b into master Nov 28, 2018
@breznak breznak deleted the eigen_speed2 branch November 28, 2018 13:17
@breznak
Copy link
Member Author

breznak commented Nov 28, 2018

@dkeeney @ctrl-z-9000-times please consider this header library if you deal with some large vectors/matrices (dense or sparse) or where speed should be an issue. Or at least comment the place so we can use it later.

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.

None yet

2 participants