tinygrad icon indicating copy to clipboard operation
tinygrad copied to clipboard

Fix argfix

Open Andriamanitra opened this issue 1 year ago • 1 comments

argfix for 0 arguments got broken in #987, this fixes it and adds a couple of tests to make sure it keeps working

the author of the change says it was "statistical optimization" (https://github.com/tinygrad/tinygrad/pull/987#discussion_r1231584626) but in my benchmarks my version is comparable (possibly even a little bit faster) across all use cases:

import timeit

def argfix_old(*x):
  if x[0].__class__ in {tuple, list}:
    try: return tuple(x[0])
    except IndexError: return tuple()
  return tuple(x)

def argfix_new(*x): return tuple(x[0]) if x and x[0].__class__ in (tuple, list) else x

N = 1_000_000
for argfix in (argfix_old, argfix_new):
    print(argfix.__name__)
    print(timeit.timeit("argfix(1, 2, 3)", number=N, globals={"argfix": argfix}))
    print(timeit.timeit("argfix(1)", number=N, globals={"argfix": argfix}))
    print(timeit.timeit("argfix((1,2,3))", number=N, globals={"argfix": argfix}))
    print(timeit.timeit("argfix([1,2,3])", number=N, globals={"argfix": argfix}))
argfix_old
0.13264382199849933
0.12719697097782046
0.1347857450018637
0.16105678299209103

argfix_new
0.10527223203098401
0.1038119489676319
0.11646945198299363
0.1526102180359885

I also included a couple of simplifications/optimizations that I ran into while reading the code

Andriamanitra avatar Jul 11 '23 07:07 Andriamanitra

Changes made in tinygrad/:

------------------------------------------------------------
files                             insertions       deletions
------------------------------------------------------------
tinygrad/graph.py                          1               1
tinygrad/helpers.py                        1               5
tinygrad/ops.py                            1               1
------------------------------------------------------------
total                                      3               7
------------------------------------------------------------
lines added in the tinygrad folder: -4

tinyb0t avatar Jul 11 '23 07:07 tinyb0t

"I also included a couple of simplifications/optimizations that I ran into while reading the code"

This makes the PR a lot harder to review. If this is a bugfix, make it just the bugfix + tests.

geohot avatar Jul 11 '23 22:07 geohot

@Andriamanitra I also recently came across this as an issue. You still working on this or should I create a PR with my fix and tests?

crnsh avatar Jul 26 '23 19:07 crnsh

@karan0handa I have no plans to finish this PR. You're welcome to use my code/tests as a starting point for a new PR if you want (no attribution needed).

Andriamanitra avatar Jul 27 '23 08:07 Andriamanitra