[Breaking Change] update rulinalg to v0.3.0

[tafia]

Adds BaseMatrix and BaseMatrixMut about everywhere.

I benchmarked timing the examples. I didn't notice a regression but better to double check on another system.

Once merged, it'll probably open the door to new optimizations now that slices have much more fn than before.

[tafia]

I suppose you've probably started this on your side, but in case ...

[AtheMathmo]

I wanted to get around to this but didn't have time - thank you so much!

Glad to hear there were no regressions on your end. I'll try it out myself either later tonight or tomorrow and merge if everything looks good (I can't see any reason why this wouldn't be the case).

[AtheMathmo]

Also - do you think this should produce a version bump to 0.5.0? It seems sensible but in practice probably not necessary. The core functionality of rusty-machine shouldn't be broken but if anyone is extending the library this will break. I imagine that right now this probably isn't likely...

[tafia]

We reexport BaseMatrix and removed BaseSlice in the prelude so this is a breaking change ... and so it should be published so people can adapt.

On the other hand, we should probably wait for having some benches BEFORE merging this so we can track perf regressions.

[AtheMathmo]

I agree with the above. However I'll add in a couple other small breaking changes before doing the release.

[AtheMathmo]

I ran the benchmarks with rulinalg 0.3.0 and saw no regressions! I think it would be best for you to confirm on your machine too - but I'm feeling pretty confident about this PR.

[tafia]

Sorry for the late answer.

I rebased on master and fixed the bench.

Unfortunately, I actually see a small regression (5% when training nnet).
Can someone else check on his side?

[AtheMathmo]

Sorry for the late answer.

No problem at all!

I saw a negligible regression on my end. I'll try again later - if the problem persists I guess we have to do some profiling to figure out the cause.

[AtheMathmo]

I ran this again using cargo-benchcmp and got the following:

 name                                     old.txt ns/iter  new.txt ns/iter  diff ns/iter  diff % 
 examples::k_means::k_means_predict       1,198,937        1,169,148             -29,789  -2.48% 
 examples::k_means::k_means_train         14,345,075       14,492,623            147,548   1.03% 
 examples::nnet::nnet_and_gate_predict    423              463                        40   9.46% 
 examples::nnet::nnet_and_gate_train      33,948,505       36,693,264          2,744,759   8.09% 
 examples::svm::svm_sign_learner_predict  4,268            4,612                     344   8.06% 
 examples::svm::svm_sign_learner_train    27,306           28,726                  1,420   5.20%

Though performance regressions aren't a huge issue at this stage. We should try to figure out what's going on. Maybe we can profile the two versions and find some low-hanging fruit?

[AtheMathmo]

First off an apology - I just released 0.3.2 of rulinalg with a breaking change. You'll need to either update this PR with the breaking changes or fix the rulinalg version (rulinalg = "=0.3.0"). The only breakage is the variance function returning a Result, it shouldn't be too rough to fix.

Onto (somewhat) better news - I did some profiling but don't really have any answers yet. I thought I'd share what I've done (and the results) to see if you can get any further.

To generate the profile:

1. Install and set up cargo profiler (or your favourite tool).
2. Generate a binary for the nnet example: cargo test --no-run --release
3. Run the profiler: cargo profiler callgrind --bin ./target/release/examples/nnet-and-gate

Here are my results:

• master callgrind
• PR callgrind
• master cachegrind
• PR cachegrind

From looking through now I haven't spotted anything obvious - maybe these profiles aren't capturing what's going on too well. I was hoping cache misses would be the issue and we just needed to specialize the relevant functions for Matrix, seems it may go deeper.

[AtheMathmo]

I've spent some more time trying to figure this out. The only possible culprit I've found is the apply function (which is used fairly liberally in neural nets). When benchmarking I found that the apply for Matrix is significantly faster if we iterate over self.data.iter_mut() instead of using self.iter_mut(). This is because self.iter_mut() returns a SliceIter. I'll make a new issue for that on rulinalg.

When I tried fixing this and using a local build I didn't really see any performance boost though.

The neural nets are pretty inefficient right now and are getting an improvement with #126 . Because of that it probably isn't worth spending much more time on this?

Bench marks for apply are below. I'm creating a matrix in the bench loop but the difference is still valid.

current:
test linalg::matrix::apply_1000_100                ... bench:     346,624 ns/iter (+/- 29,463)

improved:
test linalg::matrix::apply_1000_100                ... bench:     274,132 ns/iter (+/- 9,047)

[tafia]

Nice!
I may not have time to spend on this until October. Is it ok to wait that
long?

On 10 Sep 2016 9:52 p.m., "James Lucas" notifications@github.com wrote:

I've spent some more time trying to figure this out. The only possible
culprit I've found is the apply function (which is used fairly liberally
in neural nets). When benchmarking I found that the apply for Matrix is
significantly faster if we iterate over self.data.iter_mut() instead of
using self.iter_mut(). This is because self.iter_mut() returns a SliceIter.
I'll make a new issue for that on rulinalg.

When I tried fixing this and using a local build I didn't really see any
performance boost though.

The neural nets are pretty inefficient right now and are getting an
improvement with #126
#126 . Because of that
it probably isn't worth spending much more time on this?

Bench marks for apply are below. I'm creating a matrix in the bench loop
but the difference is still valid.

current:
test linalg::matrix::apply_1000_100 ... bench: 346,624 ns/iter (+/- 29,463)

improved:
test linalg::matrix::apply_1000_100 ... bench: 274,132 ns/iter (+/- 9,047)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#117 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHAszozPd9fDj0Dr0ip1PRBQr1FLzk-0ks5qo1C7gaJpZM4JvFL8
.

[AtheMathmo]

I'd like to get it in earlier if I'm honest - as there are a couple other PRs stalled by this. I still need to review those properly and give feedback. If those are resolved or waiting on this directly I may finish up and merge - if you don't mind of course.

[tafia]

Of course please go ahead

On 14 Sep 2016 16:02, "James Lucas" notifications@github.com wrote:

I'd like to get it in earlier if I'm honest - as there are a couple other
PRs stalled by this. I still need to review those properly and give
feedback. If those are resolved or waiting on this directly I may finish up
and merge - if you don't mind of course.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#117 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHAszj4ex867xogy88oyBALLI5aiz_9Cks5qqETSgaJpZM4JvFL8
.

[AtheMathmo]

Closing after merging #133 .