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

Updated cumsum to use K-B-N summation for float arrays. #1257

Merged
merged 1 commit into from
Sep 12, 2012

Conversation

kmsquire
Copy link
Member

@kmsquire kmsquire commented Sep 5, 2012

No description provided.

@StefanKarpinski
Copy link
Member

There was some discussion about not doing KBN by default for summation since it was found to be slower than standard summation by quite a bit on some machines. But since I believe we're doing KBN in sum by default, we should definitely do it here as well.

@StefanKarpinski
Copy link
Member

I can't find that conversation about KBN performance now, but unless I'm hallucinating, it did happen. @JeffBezanson, @ViralBShah: thoughts? Also, can you find that thread?

@timholy
Copy link
Member

timholy commented Sep 6, 2012

#1145

@StefanKarpinski
Copy link
Member

Right! Thanks, Tim.

@kmsquire
Copy link
Member Author

kmsquire commented Sep 6, 2012

Okay, I wasn't aware of #1145, only #199.

Actually, KBN is used by default for summing vectors only (see #1258), so if you wanted to change the default and/or break into fast/accurate modules, now would be a good time.

@StefanKarpinski
Copy link
Member

Yeah, we definitely need some kind of decision here. Question is which one...

@kmsquire
Copy link
Member Author

kmsquire commented Sep 6, 2012

Why, the right one, of course! ;-)

(...as opposed to the left one...)

@JeffBezanson
Copy link
Member

I picked this because I found it about 20% slower on my machine. Turns out it is 2x slower for some people. I would have decided differently if I had seen a 2x slowdown. Every other environment seems to use naive summation, and I suspect that compensated summation is not so important for real-world data. Naive summation also has the advantage of behaving the same as reduce +.
Still it's a hard decision. I hate to let performance rule everything. I'd like to believe it doesn't really matter for sum to be 2x slower.

@ViralBShah
Copy link
Member

Can we do a more systematic benchmark? 20% is certainly ok, but 2x is certainly a deal-breaker. Given that most real world uses do not care about KBN summation, it would be nice to have a kbn_sum operator that can be passed instead of + for people who do care.

@JeffBezanson
Copy link
Member

Well, I don't have a good handle on the "real world" part of it. KBN always gives a better answer, but a few ulps better is less important than the drastic differences you see in synthetic test cases.

Maybe we should consider whether people "expect" naive summation. Is 0.0 considered an acceptable answer for sum([1.0, 1e100, 1.0, -1e100])? For example it has acceptable reverse error.

@ViralBShah
Copy link
Member

Although julia gives the correct answer 2, matlab and octave both give 0.

@JeffBezanson
Copy link
Member

Defining "correct" is the problem!

@ViralBShah
Copy link
Member

Correctness is well defined, in my opinion. It is acceptability that has to be defined.

@StefanKarpinski
Copy link
Member

I think "correct" in quotations means "acceptable". But, yes, I agree, the correct answer exists and is not up for debate in these cases. I do, however, wonder if people don't actually expect naïve summation. There's also the issue that the KBN algorithm is not guaranteed to give a correct answer — it's just better than naïve.

@StefanKarpinski
Copy link
Member

Since we're currently using KBN for summation, I'm merging this so that sum and cumsum match each other.

StefanKarpinski added a commit that referenced this pull request Sep 12, 2012
Updated cumsum to use K-B-N summation for float arrays.
@StefanKarpinski StefanKarpinski merged commit 9e4e894 into JuliaLang:master Sep 12, 2012
@kmsquire
Copy link
Member Author

Just a reminder of #1258: sum is not consistent in using KBN.

@StefanKarpinski
Copy link
Member

Yeah, absolutely. I hadn't forgotten, but at least we're a little bit closer to consistency...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants