Re: [eigen] ideas about the corner types

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

2010/2/5 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
> On Fri, Feb 5, 2010 at 9:55 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
>> 2010/2/5 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>>>
>>>
>>> On Fri, Feb 5, 2010 at 4:01 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
>>> wrote:
>>>>
>>>> 2010/2/5 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>>>> >
>>>> >
>>>> > On Fri, Feb 5, 2010 at 11:43 AM, Thomas Capricelli
>>>> > <orzel@xxxxxxxxxxxxxxx>
>>>> > wrote:
>>>> >>
>>>> >>
>>>> >> Something i dont understand: why do we need to use a trick to make a
>>>> >> compile-time parameter to look like a runtime one ?
>>>> >> Why not (as is done in other parts of eigen) do corner<TopLeft>(3,3)
>>>> >> and
>>>> >> makes this explicit ?
>>>> >
>>>> > sure, ideally I would prefer your suggestion but c++ is such that the
>>>> > trick
>>>> > has the advantage of not needing the template prefix (in template code):
>>>> >
>>>> > m.corner(TopLeft,3,3)
>>>> >
>>>> > vs
>>>> >
>>>> > m.template corner<TopLeft>(3,3)
>>>>
>>>> Still, Thomas has a point since elsewhere in Eigen we're just using
>>>> template parameters. Perhaps we should be consistent. I guess that the
>>>> biggest argument in favor of my corner() proposal here is to keep the
>>>> same API as Eigen2... as you say, one could keep the current function
>>>> unchanged in              Eigen2support and move to an "officially"
>>>> templated one...
>>>>
>>>> Notice that _every_ templated method could be replaced by an
>>>> _implicitly templated one in this way_, general int parameters could
>>>> be wrapped in a CompileTime<int> empty struct, etc... so we just have
>>>> to make a choice, to use such tricks or to use plain template methods,
>>>> and be consistent with that... thanks for the reminder.
>>>
>>> right.
>>>
>>> there is also a naming issue to add Bottom, Top as we cannot reuse corner
>>> for that.
>>>
>>> So let me suggest a completely different API for all the sub matrices:
>>>
>>> m(Bottom(rows))
>>> m(Bottom<Rows>())
>>> m(TopLeft(rows,cols))
>>> m(TopLeft<Rows,Cols>())
>>> m(Block(i,j,rows,cols))
>>> m(Block<Rows,Cols>(i,j))
>>>
>>> if TopLeft, Bottom, Block (yes there is a name clash with the Block expr but
>>> that's a detail), etc. inherit the same base class (CRTP powered), we can
>>> have a single overload of operator() for these cases. The computation of the
>>> actual block parameters would be respectively done by the Bottom, TopLeft,
>>> Block, etc. classes. So it seems to me that would even simplify the
>>> implementation.
>>>
>>> This way we could even add Ranges:
>>> m(Range(i_start,i_end), Range(j_start,j_end))
>>>
>>> and Indexes (this one requires a special expression, but it's just to show
>>> the API consistency).
>>> vec(Indexes(some_vector_of_integer))
>>>
>>> This is just a rough idea which is btw closer to what you would do with
>>> MatLab.
>>>
>>> Let's see our your reaction...
>>
>> Why not, but not now. Let's do that after 3.0. I thought about similar
>> changes but I would like to consider them an additional feature to add
>> in the future. Even if that means that we eventually have 2 concurrent
>> syntaxes for blocks. That wouldn't even necessarily be a bad thing, as
>> both approaches have their advantages.
>>
>> Notice one thing: for people who dont 'using namespace Eigen', the
>> approach that you describe is very wordy, because it requires a lot of
>> Eigen:: all over the place.
>
> very good point, and this also apply to the current corner API.

Oh right.

>
>> On the other hand, it is of course very
>> attractive. So i propose to treat it as a new feature for 3.1.
>>
>> For now, let's stick to minimal changes. Right now we have a little
>> problem: corner() doesn't guarantee that it resolves the problem at
>> compile time. Let's just fix this problem:
>>
>> Two solutions:
>>  1) the solution that I proposed. It's API stable (or essentially so).
>>  2) keep corner(TopLeft, m,n) as deprecated or in Eigen2Support and
>> introduce template corner<TopLeft>(m,n).
>>
>> Also about this:
>>
>>> there is also a naming issue to add Bottom
>>
>> How about avoiding the too polluting names Bottom, Top, Left, Right
>> (especially Left and Right are super polluting) and instead do
>> BottomHalf, LeftHalf, ...
>
> hm.. and what about getting rid of corner() and instead use a set of
> individual functions:
>
>
> m.topLeft()
> ...
> m.bottom()
> m.left()
>
> No namespace verbosity or pollution. Regarding the implementation,
> corner is currently a big switch so I don't really see any drawback,
> but feel free to correct me.

The telepathic link is working at full bandwitdh. I was thinking the
same yesterday night and forgot to mention it in my email. The right
approach is definitely separate methods, like you show.

The biggest reason for grouping them in a single corner() method was,
you know, 2 years ago, we were priding ourselves in the minimalistic
aspect of our API: it had as few methods as possible. corner() allowed
to not legthen too much the list of methods in the API docs. That's
all. Fast forward to 2010, Eigen has many more methods, and that is
for the better as the advantages outweight the inconvenients by very
far, so why not add separate methods for that.

This leaves 1 question: should corner be kept as deprecated or just
moved to Eigen2Support?

Benoit

>
>
> gael
>
>> Benoit
>>
>>
>>>
>>> gael
>>>
>>>>
>>>> Benoit
>>>>
>>>> >
>>>> > gael.
>>>> >
>>>> >>
>>>> >> (The trick could still be used in the eigen2 compatibility stuff to
>>>> >> keep
>>>> >> the old api)
>>>> >>
>>>> >> Thomas
>>>> >> --
>>>> >> Thomas Capricelli <orzel@xxxxxxxxxxxxxxx>
>>>> >> http://www.freehackers.org/thomas
>>>> >>
>>>> >> In data venerdì 05 febbraio 2010 10:45:21, Benoit Jacob ha scritto:
>>>> >> > >> Currently matrix.corner(TopLeft,3,3) takes TopLeft as a runtime
>>>> >> > >> parameter. This is based around the assumption that the compiler
>>>> >> > >> resolves this at compile time. In practice that seems to work
>>>> >> > >> although
>>>> >> > >> I only checked GCC. Here's an idea to resolve this at runtime: let
>>>> >> > >> TopLeft be an object of type CornerType<TopLeft_t>, etc.. taking a
>>>> >> > >> common template CornerType type allows to write the function only
>>>> >> > >> once, yet have a guarantee that that stuff resolves at compile
>>>> >> > >> time.
>>>> >> > >
>>>> >> > > Sounds good, and that does not change the API right ?
>>>> >> >
>>>> >> > No API change, assuming that nobody was perverse enough to actually
>>>> >> > pass a runtime variable, whose value actually isn't known at compile
>>>> >> > time, as the corner type.
>>>> >>
>>>> >>
>>>> >
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>
>
>