Re: [eigen] ideas about the corner types

[Manoj Rajagopalan <rmanoj@xxxxxxxxx>]

On Thursday 22 April 2010 02:12:58 pm Benoit Jacob 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.
> >
> >> 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.

I might be a little late in joining this discussion but I would find utility 
functions to extract a block of rows or columns from the middle useful in 
addition to the corners. For example,

  for(int i=0; i<n; i+=BLOCK) {
      matrix.rows(i,i+BLOCK-1) = ...; // all cols
      matrix.cols(i,i+BLOCK-1) = ...; // all rows
   }

These constructs are useful in block-partitioned matrix arithmetic, esp. in 
solution of PDEs on a spatial grid where mesh-points in spatial "slices" are 
related only to nearest neighbours in-slice and in immediately neighbouring 
slices.

If the DenseBase<>::rows() and ::cols() names are potentially confusing then 
I'd suggest something like ::rowBlock() or colBlock()

Thanks,
Manoj