On 05 November, 2014 - Linus Torvalds wrote: > On Wed, Nov 5, 2014 at 1:54 PM, Anton Lundin <[email protected]> wrote: > > The basic problem was that for gases containing more than 2147483648 ml > > of nitrogen the calculations overflowed. This changes the code into > > using floating point math for that calculation which will be more > > accurate to. > > Ugh. So many problems with this patch. > > I agree that we do want to do part of it in floating point, but I > disagree with where you do it. > > Doing the "1000 - get_he()" and "1000-o2" in floating point is just > silly, since that's pure integer math, with no chance of overflow or > even inexact. > > And then, you really should use "rint()" to turn the floating point > value back into an integer value with proper rounding. Yeah, it > doesn't really matter when we talk "mliter" if we're off by one, but > it's the right thing to do. > > Finally, while you do too *much* fp in the first line, you then don't > do *enough* floating point in the second one. You still do the > "vol.mliter*he" as an integer multiplication, which can overflow, and > do only the division as a FP value, which is pointless the way you do > it, since without rounding, you might as well just do an integer > divide in the first place. *With* rounding, it actually makes sense as > a floating point divide, but you don't have that "rint()", so you not > only don't avoid the overflow, the one FP op you do use is pointless. > > So I think it should be something like > > air_vol = (vol.mliter * 1000.0) * (1000 - get_he(&mix) - > get_o2(&mix))) / (1000 - o2_in_topup); > he_vol = (double) vol.mliter * get_he(&mix) / 1000.0; > > air.mliter = rint(air_vol * 1000); > he.mliter = rint(he_vol * 1000); > o2->mliter = vol.mliter - he->mliter - air.mliter; > > so that *all* the multiples and divides get done as floating point, > but the exact integer subtractions don't, and the final integer > assignment is the properly rounded one. > > FP is hard. But let's try to get it right. > > Also, that whole "1000 - get_he(&mix) - get_o2(&mix)" might possibly > be good as a "get_n2(mix)" helper function. No? Although in this > context, maybe that doesn't make sense. >
I was obviously to tired last night to produce useful code or commit messages, and thanks for catching this. I'll send out a v2 tonight (morning here now) unless you who already solved it would like to do it? //Anton -- Anton Lundin +46702-161604 _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
