Let's get some preliminary problems out of the way before we discuss efficiency — and there is much efficiency to be gained.
Random number generator
You appear to be using the advice and public-domain code from David Jones for your pseudo-random number generator. Basically,
init_KISS()
seeds the pseudorandom number generator
JKISS()
generates a random unsigned int
uni_dblflt()
generates a random double
between 0 and 1 using a uniform distribution.
It would have been nice to note that information in the question.
Declutter your main()
by moving this code out into a function with a self-documenting name.
void initialize_prng()
{
init_KISS();
for (int discard = 0; discard < 100000; discard++)
{
JKISS(); /* waste the first 100000 random numbers */
uni_dblflt();
}
}
Proliferation of variables
First, let the compiler alert you to problems. Compiling with warnings turned on, we see problems about how variables are being used.
$ gcc -Wall -c yule.c
yule.c:13:9: warning: unused variable 't' [-Wunused-variable]
int t,count,i,j,k,ii,jj,kk;
^
yule.c:13:29: warning: unused variable 'kk' [-Wunused-variable]
int t,count,i,j,k,ii,jj,kk;
^
2 warnings generated.
For that matter, r
is assigned but never used, so it could also be eliminated. v
is just a constant 1, and can also be eliminated.
The overall problem here is that you have a proliferation of variables that makes the code hard to follow. Ever since C99, you can declare variables anywhere inside a function, not just at the top. (It's now 2014 — surely it's OK to use C99?) Best practice is to declare your variables as close to the point of use as possible, as it improves readability, decreases mental workload, and puts variables in a tighter scope to avoid silly mistakes.
Other matters of style
An int
is only guaranteed to hold 16 bits, or numbers up to 32767. In practice, your code will work because an int
will be 32 bits on any modern machine. However, if you want to be certain that you can store 106, strictly speaking, you should use a long
.
Think about readability when using loop-counting variables. For example, for(j=10;j<desval;j++)
is notionally a continuation of for(i=0;i<10;i++)
, just with a different loop body. Therefore, I would use i
as the counter for the second loop as well.
I don't see any need here for #include <math.h>
and #include <time.h>
.
Instead of zeroing the arrays manually, use calloc()
, which zeroes out the memory it allocates.
You allocate one more element than you need for each array, as if making a zero-terminator element at the end. That's something that one would do with NUL-terminated strings and some arrays of pointers. For this problem, a zero-terminator is useless, since 0.0 is a legal value that can't act as a sentinel. Therefore, the code would be less puzzling if you dropped the pretense of having a zero-terminator.
Declutter your main()
function further by extracting the printing loop into its own function.
Efficiency
As @llewelly noted, you are doing unnecessary work to sum the arrays repeatedly. How bad is it?
for(j=10;j<desval;j++)
{
…
for(ii=0;ii<desval;ii++)/*Evaluate the probability total cutoffs*/
{
ptot[ii]=array_sum(p,ii);
}
…
}
Three nested loops makes it O(n3). When n = 106, that's significant!
@llewelly suggests removing the innermost array_sum()
, to make it O(n2). We can actually do much, much better than that.
First, note that the p
array is a useless intermediary between bins
and ptot
.
Next, consider combining bins
and ptot
into a single array. Suppose that instead of using bins
to store the number of balls in each element, we had a cumulative_bins
that stored the cumulative number of balls from bin 0 to bin i. Reconstructing what bins
would have held is trivial: just subtract the predecessor from cumulative_bins[i]
. Reconstructing what ptot
would have held is also easy: divide cumulative_bins[i]
by cumulative_bins[desval - 1]
, where cumulative_bins[desval - 1]
is the total number of balls in the system. (Note that this cumulative_bins[]
should be discrete, not floating-point.)
If you try to implement that idea, you'll find that reading the cumulative number of balls in bins 0 to i is an O(1) operation, adding one ball to a bin is an O(n) operation, and finding the index of a cumulative count is O(n). Fortunately, there is a nice data structure that can improve on that: a binary indexed tree, also known as a Fenwick Tree, which can do all three operations in O(log n) time.
Solution
This program performs the job in O(n log n) time, in contrast to your original O(n3). Excluding the time it takes to print the output, it completes a simulation of n = 106 bins in about half a second.
long array_get_cumulative(const long fenwick_tree[], long idx)
{
long sum = 0;
for (; idx >= 0; idx = (idx & (idx + 1)) - 1)
{
sum += fenwick_tree[idx];
}
return sum;
}
void array_incr(long fenwick_tree[], long idx, size_t size, long incr)
{
for (; idx < size; idx |= (idx + 1))
{
fenwick_tree[idx] += incr;
}
}
long array_find_cumulative(long fenwick_tree[], long cumFre, size_t size)
{
long idx = 0, tIdx, bitMask;
for (bitMask = 1; size & ~((bitMask << 1) - 1); bitMask <<= 1) {}
for (; bitMask && ((tIdx = idx + bitMask) < size); bitMask >>= 1)
{
if (cumFre >= fenwick_tree[tIdx - 1])
{
// if current cumulative frequency is equal to cumFre,
// we are still looking for higher index (if exists)
idx = tIdx;
cumFre -= fenwick_tree[tIdx - 1];
}
}
return idx - (cumFre == 0);
}
void array_print(long fenwick_tree[], size_t size)
{
for (long i = 0, preceding_cumulative = 0; i < size; i++)
{
long cumulative = array_get_cumulative(fenwick_tree, i);
printf("%ld\n", cumulative - preceding_cumulative);
preceding_cumulative = cumulative;
}
}
int main()
{
long n = 1000000L;
initialize_prng();
/* Urns, keeps track of cumulative "balls" in each bin (including all preceding bins) */
long *bins = calloc(n, sizeof(long));
if (bins == NULL)
{
fprintf(stderr, "Out of memory\n");
exit(1);
}
for (long i = 0; i < 10; i++) /* seed 10 initial bins with 1 "ball" each */
{
array_incr(bins, i, n, 1);
}
for (long i = 10; i < n; i++)
{
long ball_count = array_get_cumulative(bins, n - 1);
double u = uni_dblflt();
long j = array_find_cumulative(bins, u * ball_count, n);
array_incr(bins, j, n, 1);
array_incr(bins, i, n, 1);
}
array_print(bins, n);
free(bins);
return 0;
}
There may be off-by-one errors in array_find_cumulative()
— I'll leave it to you to work them out.