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

Make random access queue sampling code cleaner #309

Merged
merged 1 commit into from
Oct 9, 2018

Conversation

prabhatnagarajan
Copy link
Contributor

We have previously overridden the getitem operator. However, the sampling method doesn't make use of this override, and instead replicates much of the code in that method. This commit does change the semantics of what occurs in practice. However, due to the uniformity of sampling, in expectation it does the same thing. In a nutshell, both the previous code as well as this commit implement uniform sampling. However, the indexes that are chosen are different between the previous code and this one.

@toslunar
Copy link
Member

toslunar commented Oct 9, 2018

I measured the time to execute

def f():
    q = RandomAccessQueue(maxlen=10000)
    for _ in range(100000):
        q.append(1)
        if len(q) > 1000:
            q.sample(64)

in #131 (comment).

Before the PR

In [2]: %timeit q.f()
6.62 s ± 68.3 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

After the PR

In [2]: %timeit q.f()
10.4 s ± 270 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

@toslunar
Copy link
Member

toslunar commented Oct 9, 2018

It's OK because 40 us per iteration is not big.

@toslunar toslunar changed the title makes random access queue sampling code cleaner, changes the semantic… Make random access queue sampling code cleaner Oct 9, 2018
@toslunar toslunar merged commit 194cd14 into chainer:master Oct 9, 2018
@muupan muupan added this to the v0.5 milestone Nov 13, 2018
@prabhatnagarajan prabhatnagarajan deleted the sample branch March 14, 2019 12:21
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.

3 participants