[Piglit] [PATCH 2/4] arb_shader_precision: add framework for calculating tolerances for complex functions

[Dylan Baker]

On Fri, Feb 20, 2015 at 06:31:34PM -0500, Ilia Mirkin wrote:
> On Thu, Feb 19, 2015 at 1:06 AM, Micah Fedke
> <micah.fedke at collabora.co.uk> wrote:
> > +def _gen_tolerance(name, rettype, args):
> > +    """Return the tolerance that should be allowed for a function for the
> > +    test vector passed in.  Return -1 for any vectors that would push the
> > +    tolerance outside of acceptable bounds
> > +    """
> > +    if name in simple_fns:
> > +        if name == 'op-mult' or name == 'op-assign-mult':
> 
> Seems like this should be outside of the if. So like
> 
> if name == 'op-mult' or name == ...:
> elif name in simple_fns:
> elif name in compelx_fns:
> 
> > +            x_type = glsl_type_of(args[0])
> > +            y_type = glsl_type_of(args[1])
> > +            if x_type.is_vector and y_type.is_matrix:
> > +                mult_func = _vec_times_mat_ref
> > +            elif x_type.is_matrix and y_type.is_vector:
> > +                mult_func = _mat_times_vec_ref
> > +            elif x_type.is_matrix and y_type.is_matrix:
> > +                mult_func = _mat_times_mat_ref
> > +            else:
> > +                return simple_fns[name]
> > +            ret = _analyze_ref_fn(mult_func, args)
> > +            return -1.0 if any(ret['badlands']) else map(float,
> > ret['component_tolerances'])
> > +        else:
> > +            return simple_fns[name]
> > +    elif name in complex_fns:
> > +        if name in componentwise_fns:
> > +            ret = {'errors':[], 'badlands':[], 'component_tolerances':[]}
> 
> Is there some sort of advantage to keeping these in a dict?
> 
> > +            for component in range(rettype.num_cols*rettype.num_rows):
> 
> xrange. never use range (except when you really just need the array,
> which is... never. or python3, which this isn't).
> 
> > +                current_args = []
> > +                for i, arg in enumerate(args):
> > +                    current_args.append(arg[component%len(arg)] if
> > _len_any(arg) > 1 else arg)
> > +                rettmp = _analyze_ref_fn(complex_fns[name], current_args)
> > +                ret['errors'].extend(rettmp['errors'])
> > +                ret['badlands'].extend(rettmp['badlands'])
> > + ret['component_tolerances'].extend(rettmp['component_tolerances'])
> > +        else:
> > +            ret = _analyze_ref_fn(complex_fns[name], args)
> > +        return -1.0 if any(ret['badlands']) else map(float,
> > ret['component_tolerances'])
> > +    else:
> > +        return 0.0
> 
> This function is confusing. It sometimes returns floats, other times
> arrays. What all these functions are doing is somewhat clever... can
> you better describe the arguments and return values for these helpers
> (especially this one)? And also comment things more? This is an
> *extremely* dense function, I think it does a number of non-obvious
> things. Would be nice to have comments explaining what you're trying
> to account for or what some case is trying to handle.
> 
> Also, not sure if Dylan addressed this, but this is also unusual
> spacing. a * b, not a*b, etc. This is how all the C code is written,
> should carry over into Python as well.

I didn't, but yes. PEP8 whenever it is reasonable. Spaces around math
operators always in the python code please.

> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150220/bf41a587/attachment.sig>