Re: [eigen] news from the LU / rank / solving API front

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

2009/10/18 Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>:
> Hey Benoit,
>
> The class looks good! I have only very few comments. First, I am wondering
> why useThreshold is returning a reference to the LU object and that actually
> only according to the signature since the current implementation returns
> nothing.

OK, it was a bug that i omitted the return statement; my intention was
to allow chained calls like

  lu.useThreshold(x).rank()

but indeed that is perhaps not a good idea as that syntax would
suggest that the threshold value is only used for that call. So I
think that the name useThreshold was a bit bad. How about renaming it
to setThreshold (so it's clear that it's a permanent change) and then
allow

 lu.setThreshold(x).rank()

?

> I would simply make the return type void. Second, for the threshold
> retrieval, I woul make only 'RealScalar threshold() const' public and move
> the defaultThreshold() method to the private section or even consider
> removing it. When the default threshold is set, the thresold() function does
> return it - in case it is not set, I don't see why a 'public' user should be
> interested in it.

OK, that makes sense.

>
> Finally, I am wondering why all the variables are 'protected'. Do we expect
> somebody to inherit from LU? If that were the case it should have a virtual
> destructor. There is not harm in declaring the variables as protected - I
> was simply wondering.

It's just a habit that I have, to make variables protected, just in
case some day someone wants to inherit. It actually happens in
practice when people want to adjust the API e.g. to make porting
easier (I know that Thomas subclassed Matrix in the past).

Why should the destructor be virtual here? Is it important, in our
context, to allow referring to Derived objects as objects of the base
(here LU) type?

Cheers,
Benoit