[PATCH] Convert volume calculations into floating point

Anton Lundin glance at acc.umu.se
Wed Nov 5 22:49:54 PST 2014


On 05 November, 2014 - Linus Torvalds wrote:

> On Wed, Nov 5, 2014 at 1:54 PM, Anton Lundin <glance at acc.umu.se> 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


More information about the subsurface mailing list