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.
>
>> 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:
>

I chose slightly longer method names to avoid any risk of confusion / pollution.
>
> m.topLeft()

it's m.topLeftCorner(int r, int c)
etc.

> ...
> m.bottom()
> m.left()

m.topRows(n)
m.bottomRows(n)
m.leftCols(n)
m.rightCols(n)

Benoit

>
> 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.
>
>
> 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.
>>>> >>
>>>> >>
>>>> >
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>
>
>