Re: [eigen] ideas about the corner types

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

On Fri, Feb 5, 2010 at 11:08 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
> 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?

we can keep it as deprecated and move it to Eigen2Support just before
the first beta.

gael

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