Re: [Numpy-discussion] Using logfactorial instead of loggamma in random_poisson sampler

2021-03-08 Thread zoj613
What do you think is the explanation for that? I had assumed that using a
lookup table would be faster considering that the loggam implementation has
loops and makes calls to elementary functions in it.



--
Sent from: http://numpy-discussion.10968.n7.nabble.com/
___
NumPy-Discussion mailing list
NumPy-Discussion@python.org
https://mail.python.org/mailman/listinfo/numpy-discussion


Re: [Numpy-discussion] Using logfactorial instead of loggamma in random_poisson sampler

2021-03-08 Thread Kevin Sheppard
I did a quick test and using random_loggam was about 6% faster than using
logfactorial (on Windows).

Kevin


On Sun, Mar 7, 2021 at 2:40 AM Robert Kern  wrote:

> On Sat, Mar 6, 2021 at 1:45 PM Warren Weckesser <
> warren.weckes...@gmail.com> wrote:
>
>> At the time, making that change was not a high priority, so I didn't
>> pursue it. It does make sense to use the logfactorial function there,
>> and I'd be happy to see it updated, but be aware that making the
>> change is more work than changing just the function call.
>>
>
> Does it make a big difference? Per NEP 19, even in `Generator`, we do
> weigh the cost of changing the stream reasonably highly. Improved accuracy
> is likely worthwhile, but a minor performance improvement is probably not.
>
> --
> Robert Kern
> ___
> NumPy-Discussion mailing list
> NumPy-Discussion@python.org
> https://mail.python.org/mailman/listinfo/numpy-discussion
>
___
NumPy-Discussion mailing list
NumPy-Discussion@python.org
https://mail.python.org/mailman/listinfo/numpy-discussion


Re: [Numpy-discussion] Using logfactorial instead of loggamma in random_poisson sampler

2021-03-06 Thread Robert Kern
On Sat, Mar 6, 2021 at 1:45 PM Warren Weckesser 
wrote:

> At the time, making that change was not a high priority, so I didn't
> pursue it. It does make sense to use the logfactorial function there,
> and I'd be happy to see it updated, but be aware that making the
> change is more work than changing just the function call.
>

Does it make a big difference? Per NEP 19, even in `Generator`, we do weigh
the cost of changing the stream reasonably highly. Improved accuracy is
likely worthwhile, but a minor performance improvement is probably not.

-- 
Robert Kern
___
NumPy-Discussion mailing list
NumPy-Discussion@python.org
https://mail.python.org/mailman/listinfo/numpy-discussion


Re: [Numpy-discussion] Using logfactorial instead of loggamma in random_poisson sampler

2021-03-06 Thread zoj613
Ah, I had a suspicion that it was to preserve the random stream but wasn't
too sure. Thanks for the clarification.



--
Sent from: http://numpy-discussion.10968.n7.nabble.com/
___
NumPy-Discussion mailing list
NumPy-Discussion@python.org
https://mail.python.org/mailman/listinfo/numpy-discussion


Re: [Numpy-discussion] Using logfactorial instead of loggamma in random_poisson sampler

2021-03-06 Thread Warren Weckesser
On 3/6/21, zoj613  wrote:
> Hi All,
>
> I noticed that the transformed rejection method for generating Poisson
> random variables used in numpy makes use of the `random_loggam` function
> which directly calculates the log-gamma function. It appears that a
> log-factorial lookup table was added a few years back which could be used
> in
> place of random_loggam since the input is always an integer. Is there a
> reason for not using this table instead? See link below for the line of
> code:
>
> https://github.com/numpy/numpy/blob/6222e283fa0b8fb9ba562dabf6ca9ea7ed65be39/numpy/random/src/distributions/distributions.c#L572
>
> Regards
> Zolisa
>

Hi Zolisa,

In the pull request where the C function logfactorial was added
(https://github.com/numpy/numpy/pull/13761), I originally modified the
Poisson code to use logfactorial as you suggest, but Kevin (@bashtage
on github) pointed out that the change could potentially alter the
random stream for the legacy version. Making the change requires
creating separate C functions, one for the legacy code that remains
unchanged, and one for the newer Generator class that would use
logfactorial.  You can see the comments here (click on "Show
resolved"):

https://github.com/numpy/numpy/pull/13761#pullrequestreview-249973405

At the time, making that change was not a high priority, so I didn't
pursue it. It does make sense to use the logfactorial function there,
and I'd be happy to see it updated, but be aware that making the
change is more work than changing just the function call.

Warren

>
>
> --
> Sent from: http://numpy-discussion.10968.n7.nabble.com/
> ___
> NumPy-Discussion mailing list
> NumPy-Discussion@python.org
> https://mail.python.org/mailman/listinfo/numpy-discussion
>
___
NumPy-Discussion mailing list
NumPy-Discussion@python.org
https://mail.python.org/mailman/listinfo/numpy-discussion


[Numpy-discussion] Using logfactorial instead of loggamma in random_poisson sampler

2021-03-06 Thread zoj613
Hi All,

I noticed that the transformed rejection method for generating Poisson
random variables used in numpy makes use of the `random_loggam` function
which directly calculates the log-gamma function. It appears that a
log-factorial lookup table was added a few years back which could be used in
place of random_loggam since the input is always an integer. Is there a
reason for not using this table instead? See link below for the line of
code:

https://github.com/numpy/numpy/blob/6222e283fa0b8fb9ba562dabf6ca9ea7ed65be39/numpy/random/src/distributions/distributions.c#L572

Regards
Zolisa



--
Sent from: http://numpy-discussion.10968.n7.nabble.com/
___
NumPy-Discussion mailing list
NumPy-Discussion@python.org
https://mail.python.org/mailman/listinfo/numpy-discussion