| Re: [eigen] Specializing max_coeff_visitor for some number types |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/eigen Archives
]
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.
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.
* 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 also would only apply it to the development branch.
Of course.
Thanks for the comments,
--
Marc Glisse