Re: [eigen] Specializing max_coeff_visitor for some number types

[Marc Glisse <marc.glisse@xxxxxxxx>]

On Mon, 26 May 2014, Christoph Hertzberg wrote:

> On 19.05.2014 22:14, Marc Glisse wrote:
> > On Mon, 19 May 2014, Christoph Hertzberg wrote:
> >
> > > On 18.05.2014 15:44, Marc Glisse wrote:
> > > > I changed the CGAL testcase because it is important that only 0 have a
> > > > score of 0 (the interval [0,1] should have a non-zero score). It is not
> > > > optimal (I would like [0,1]>[0,0] but [1,2]<[1,1]) but good enough (some
> > >
> > > Ok, using [0,1] as pivot may be better than [-0,0] because the inverse
> > > is [1,inf] as opposed to [-inf,inf]. But I think [-1,1] will be worse
> > > than [0,0] in general.
> >
> > That's not my reason: if we pick [0,0] as the "best", eigen will believe
> > the column / sub-matrix is 0 and not use this coeff as a pivot (the
> > inverse doesn't matter). So I need to make sure I don't return a score
> > of 0 unless I am sure all coeffs are [0,0]. If I pick [0,1] as the best,
> > the test "== Score(0)" will throw (default behavior of CGAL intervals)
> > and restart the whole computation with an exact number type, which seems
> > fine to me.
>
> I still think your matrix is "possibly zero" as soon as a zero is inside all 
> intervals -- no matter if these intervals are [0,0] or [0,1000] or 
> [-1000,1000].

My issue is making sure that Eigen doesn't think the matrix is "certainly 
zero" when it is only "possibly zero". Because of the way CGAL works, when 
we ask for the rank of a matrix, we want either an answer that is certain 
or no answer at all (exception). Anything else will break the programs 
using it.

> > > Some things about your patch I'm not sure about:
> > > * Introducing a user-level function "cwiseScore" which I doubt will
> > >   have a purpose aside from pivoting.
> >
> > I did it this way because I could just copy the cwiseAbs function, I am
> > not familiar enough with the code to understand how it is working. Is
> > there a convenient way to achieve the same thing without introducing a
> > user-level function? (I'll see if I can find one, but a hint would be
> > welcome). We can also not document the function (or mark the
> > documentation as internal or something) if that helps.
>
> You can directly call MatrixBase::unaryExpr:
> http://eigen.tuxfamily.org/dox-devel/classEigen_1_1MatrixBase.html#a23fc4bf97168dee2516f85edcfd4cfe7
>
> Simply pass a scalar_score_coeff_op object:
> >  biggest_in_corner = m_lu.bottomRightCorner(rows-k, cols-k)
> >                          .unaryExpr(scalar_score_coeff_op<Scalar>())
> >                          .maxCoeff(&row_of_biggest_in_corner, 
> > &col_of_biggest_in_corner);
>
> should work (not tested).

Looks great, I'll try that. Thanks for the help!

> > > * Would there be a way to avoid the extra coeff-access and abs()
> > >   calculation in case score(Scalar)==abs(Scalar)? I guess the
> > >   performance impact is very minor, but it's avoidable.
> >
> > Sure, just a bit of meta-programming, I'll do that when I get the time.
> > I thought the extra ugliness might be taken against the patch, so I
> > didn't do it in the first version.
>
> I don't know what would be the least ugly and least intrusive way. I'm afraid 
> that
>  internal::is_same<scalar_abs_op, scalar_score_coeff_op>::value
> will not work.

More like std::is_base_of in this case.

> Maybe add another flag to
>  functor_traits<scalar_score_coeff_op<Scalar> >?

I was thinking of adding some typedef (or static enum or whatever) in the 
default implementation of scalar_score_coeff_op and testing for the 
existence of that typedef. Unless some user defines his own 
scalar_score_coeff_op<Type> specialization by deriving from 
scalar_score_coeff_op<OtherType>, it should detect if the user specialized 
scalar_score_coeff_op without the risk of forgetting to update 
functor_traits. Does that seem ok?

> I will apply your patch (as soon as it's finished), if no opposing reactions 
> come until then.

Thanks :-)

--
Marc Glisse