PERF: Updated andrews_curves to use Numpy arrays for its samples
State: closed

Hello,

I hope I've followed the contribution guidelines correctly, but am happy to change things if necessary.

I noticed that andrews_curves doesn't make use of numpy arrays in what I thought was a sensible use case: for generating its samples.

I added a test which uses variable length random data, so that I could check the timing changes between the numpy and non-numpy versions and found the following (rough data):

| Length of data | Time / s (non-numpy) | Time / s (numpy) |
| --- | --- | --- |
| Without test | 3.43 | 3.84 |
| 10 | 4.69 | 4.21 |
| 100 | 6.58 | 5.20 |
| 1000 | 20.67 | 16.41 |
| 10000 | 162.17 | 125.37 |

The test adds some overhead (though it is decorated with `@slow`), so I'm happy to amend the commit and remove it. Otherwise, the changes seem to have resulted in a small speed up, which becomes more important for larger data (my original motivation, since I was trying to do it with a 100k x 5 dataframe).

Thanks,

Kyle


Labels:
name: Visualization
description: plotting
name: Performance
description: Memory or execution speed performance

Comments:
lgtm. @TomAugspurger ?

Yeah. @khs26 could you add an item to the release notes under enhancements? `doc/source/whatsnew/v0.17.0`

@khs26 are those times in seconds?

1) I'll add changes to the release notes.

2) Yeah, the times are in seconds and rough timing for the whole test (as opposed to just my changes). I'm still getting familiar with `nose` and testing generally in Python.

ok, some comments.

Let's create another issue that shows the performance implications (after this PR), and maybe you (or another brave soul) can address in the future.

I actually found a bit of time to fix it up. I'm going to update the branch shortly.

This change didn't really change things hugely. I suspected it would only make a significant difference on wide dataframes (i.e. those with a large number of coefficients). I also think it would be pushing the utility of Andrews plots as a visualisation technique. It's definitely a close thing between a slight performance increase, with slightly lessened readability.

Here are the timings:

| (length, width) of data | Time / s (non-numpy) | Time / s (first vectorisation) | Time / s (both vectorisation) |
| --- | --- | --- | --- |
| **length 10** |  |  |  |
| 10, 1 | 0.04 | 0.03 | 0.03 |
| 10, 10 | 0.07 | 0.06 | 0.07 |
| 10, 100 | 0.16 | 0.10 | 0.12 |
| 10, 1000 | 0.86 | 0.26 | 0.20 |
| **length 100** |  |  |  |
| 100, 1 | 0.29 | 0.31 | 0.25 |
| 100, 10 | 0.47 | 0.32 | 0.29 |
| 100, 100 | 1.06 | 0.55 | 0.42 |
| 100, 1000 | 7.74 | 1.59 | 1.22 |
| **length 1000** |  |  |  |
| 1000, 1 | 1.88 | 1.70 | 1.41 |
| 1000, 10 | 2.87 | 1.44 | 2.06 |
| 1000, 100 | 9.83 | 3.07 | 2.33 |
| 1000, 1000 | 75.11 | 13.48 | 10.19 |
| **length 10000** |  |  |  |
| 10000, 1 | 3.31 | 2.20 | 2.00 |
| 10000, 10 | 4.68 | 2.29 | 2.48 |
| 10000, 100 | 16.94 | 4.22 | 4.06 |
| 10000, 1000 | 149.49 | 25.25 | 19.45 |

I've left it as a separate commit for the moment, but will squash it, if we're happy with it.

can you add an asv benchmark for this? http://pandas.pydata.org/pandas-docs/stable/contributing.html#running-the-performance-test-suite

otherwise looks good.

@khs26 can you rebase / update

Sorry that took a while, been quite busy. I ran a profile on it, and it turns out that the existing changes make the slowest part be drawing in `matplotlib` (with the same/lower scaling).

I added an `asv` benchmark as well. Hopefully I got it right, it seems to run ok and I copied the basic template from timeseries plotting in `benchmarks/plotting.py`.

Also rebased the commit on top of the latest master, so it should be good to merge.

looks good. ping on green.

Travis is happy and I'm gonna leave it be now, so can merge whenever you want.

thank you sir!