Optimisation - #12
Conversation
|
Did you use AI to produce this PR ? If so, how much (100% of modifications or less ?) ? |
|
say 80% were produced by AI, but i manually checked all changes to make sure they are equivalent to the original |
|
I have mixed feelings.
For the cons :
I am not saying we shouldn't use AI (I have also mixed feelings about it, to be honest), but I know it is a powerful tool. But I believe we should do the following :
For this optimisation PR, ideally I would like that for the espm.estimators folder a pre-PR study is performed. Identifying the bottleneck in terms of computation speed. Then, perform optimisation with, if necessary, the help of AI. I am opened to discussion on the topic. |
|
I have to say, that for squared_distance the documentation is wrong. 😆 |
I am pretty sure the optimised code is equivalent btw. This script runs without panicking: import numpy as np
def original(x, y=None):
try:
x.shape[1]
except IndexError:
x = x.reshape(1, x.shape[0])
if y is None:
y = x
else:
try:
y.shape[1]
except IndexError:
y = y.reshape(1, y.shape[0])
rx, cx = x.shape
ry, cy = y.shape
if cx != cy:
raise ValueError("The sizes of x and y do not fit")
xx = (x * x).sum(axis=1)
yy = (y * y).sum(axis=1)
xy = np.dot(x, y.T)
d = abs(np.kron(np.ones((ry, 1)), xx).T + np.kron(np.ones((rx, 1)), yy) - 2 * xy)
return d / cx
def opt(x, y=None):
try:
x.shape[1]
except IndexError:
x = x.reshape(1, x.shape[0])
if y is None:
y = x
else:
try:
y.shape[1]
except IndexError:
y = y.reshape(1, y.shape[0])
rx, cx = x.shape
ry, cy = y.shape
if cx != cy:
raise ValueError("The sizes of x and y do not fit")
xx = (x * x).sum(axis=1)
yy = (y * y).sum(axis=1)
xy = np.dot(x, y.T)
d = np.abs(xx[:, np.newaxis] + yy[np.newaxis, :] - 2 * xy)
return d / cx
for c in range(1, 100):
x = np.random.random(size=(c + 1, c))
y = np.random.random(size=(c + 2, c))
assert np.array_equal(original(x, y), opt(x, y))If you give the documentation example ( I do admit that this change is non-trivial. Let me list the non-trivial changes made in this PR by commenting. But at the end of the day, I think the only change that is breaking equivalence is 6c5e939. |
| W4 = -1 * np.ones((2, brstlg_comps)) | ||
| W_brstlg = np.vstack((W3, W4)) | ||
| W = np.hstack((W_elts, W_brstlg)) | ||
| L = len(elements) |
There was a problem hiding this comment.
This is quite a big change but if u look closely it's way more elegant than the original method, and actually much easier to understand
| ranges_list = [(values[2 * i - 1], values[2 * i]) for i in range(1, num + 1)] | ||
| return ranges_list | ||
| values = np.linspace(axis.low_value, axis.high_value, num=2 * num + 2) | ||
| return values[1:-1].reshape(num, 2).tolist() |
There was a problem hiding this comment.
np.reshape can be a bit hard to understand
| for i, p in enumerate(areas_dict): | ||
| H[i, :, :] = areas_dict[p] | ||
|
|
||
| H = np.array(list(areas_dict.values())) |
There was a problem hiding this comment.
this is a pretty cool vectorisation.
| a = np.apply_along_axis(max_masked, 0, num) | ||
| val = num / 2 - denum | ||
| val[num <= 0] = -np.inf | ||
| a = np.max(val, axis=0) |
|
other than the commented changes, the rest are either trivial use of better np functions, or list/dictionary comprehension, or refractoring of common code, or etc |
|
regarding the use of AI, i generally only use AI for tasks that are not as significant, like the |
that is a good idea, we can do that in another PR. for this PR is just small changes that are equivalent. |
|
Massive speed-up by associating matrix multiplication to lower FLOPs one. Before: where
p is dominating, so the one before takes up significantly more FLOPs. Before (245.22s): After (234.27s): |
|
for this function, i realised that the documentation which mentions that |
2 participants
This is an attempt at optimising the library.
Originally, the tests finish in 334.29s (0:05:34):
Now, the tests finish in 262.92s (0:04:22):
Both ran on my machine with i5-1135G7 with 16GB ram. I have tried running on more powerful machines but the difference is much less significant.
All changes should preserve behaviour.
Will also try to add
numbaandcupycapability in another PR.Please pay particular attention to 6c5e939, which should not alter the original behaviour but it is not equivalent if called directly.