Histogram Algorithm

[mjschaible]

I was wondering if anyone had any suggestions for improvement to a logarithmic scaled histogram algoritm I wrote. It is very simple, but I think that the efficiency could be improved some.

I am using writing in Origin 7.5 (2006) C programming so please assume I don't have access to any "special" functions (i.e. anything beyond basic programming functions (while, for, do..).

void makehist()
{
        int i=0, j=0, nhist=100;
	int sum;
	vector<int> vFreq;
	double histBinLow, histBinHigh, currEng;
  	while(i<nIons)
	{
		currEng=vEng[i];
		j=0;
		while(j<nhist)
		{
			histBinLow = (1.15)^j*1/4000;
			histBinHigh = (1.15)^(j+1)*1/4000;
			if(currEng >= histBinLow && currEng < histBinHigh)
			{
				vFreq[j]=vFreq[j]+1;
				sum++;
				printf("The number of values in %f is %d \n", vBins[j], vFreq[j]);
				break;
			}
			j++;
				
		}
		i++;
	}
}

[jlb]

Let's look at the following snippet:
histBinLow = (1.15)^j*1/4000;
I see a couple of possible problems here. First do you realize that the operator^ is a bitwise operator, not the power operator.

Next the following: 1/4000 is probably not doing what you expect. This will yield zero since there are no fractions when using integral math. To force using floating point math you need to either use floating point constants or cast at least one of the constants to floating point.

[mjschaible]

Didn't know that about raising to a power...
As soon as I looked in the documentation I found the correct terminology - z= pow (double x, double y)
Thanks!

To fix the floating point issue (which oddly enough DID seem to work as I was able to create the histograms using the code as it stood) I simply added:
float range = 1/4000;

If there are any other suggestions on how to improve the number of loops that are performed or to suggest a more efficient way to search the data I would appreciate the suggestions.

Thanks!

[jlb]

To fix the floating point issue (which oddly enough DID seem to work as I was able to create the histograms using the code as it stood) I simply added:
float range = 1/4000;

Actually that is still doing integer math. You need to make at least one of the values a floating point value. There a a couple of ways to accomplish this:

double range = 1.0/4000;
double range2 = 1/4000.0;
double range3 = 1.0/4000.0;
double range4 = (double)1/4000; // Cast one or the other or both to a double.
double divisor = 1;
double range5 = divisor / 4000;

Note: I suggest you stick with doubles for your floating point numbers unless there is a very good reason to use the float type. Most of the time the extra precision is what is really needed.

Another issue I see is that you're not initializing sum before you use it, although it doesn't seem like you're actually using this variable.

[dhayden]

Technically, once you fix the power issue, I think the integers will work. Specifically, pow(1.15, j)*1/4000 is okay because it evaluates left to right ((pow(1.15,j)*1)/4000). Since pow is a double, 1 will get promoted to double. Then 4000 will get promoted in the division. If it evaluated right to left then jlb would be right.

By I'm being pedantic. Just make it clear with pow(1.15, j)/4000.0.

As for the performance, you can speed this up a lot. First, compute the upper bound of each bin and put the values in an array. For each value in vEng, do a binary search to find the correct bin number and put it there. This will run much faster.