Re: [eigen] New indexing/slicing API: almost ready to be merged

[ Thread Index | Date Index | More Archives ]

Awesome work! This is a great improvement to a great library.

Just a thought about lastN(n).

On 13 Jan 2017, at 11:43, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

The "block API" has pretty convenient shortcuts as: topRightCorner, middleRows, leftCols, etc. that ease readability and reduce redundancy. It would be nice to elegantly cover all these shortcuts with the new API. Since each dimension is now treaded independently, we only have to cover four cases corresponding to all, segment(i,n), head(n), and tail(n). The first two are well covered by all and seqN/seq. For the last two we have:

head(n) <--> seqN(0,n)

tail(n) <--> seqN(placeholders::end-n,n) or seq(placeholders::end-n,placeholders::last)

whereas "seqN(0,n)" reads OK, something as simple and frequent as taking the last n element should be easier to write/read. Here, using either seq or seqN we have to repeat ourself which is never good. So I think we should really add an alias for seqN(placeholders::end-n,n), and for symmetry, adding an alias for seqN(0,n) might be a good idea too. What about:

  firstN(n), firstN<N>, firstN<N>(n)
  lastN(n), lastN<N>, lastN<N>(n)

tail(n) <-->  seqN(last,n,-1).reverse() 
I think that the ability to index from last (with a less verbose syntax) would be a welcome addition.

Something like pesN(last, size) <—> seqN(last,n,-1).reverse()
( I know pes is fancy and probably does not make sense but maybe you have better solutions)  

This way, A. topRightCorner(rows,cols) becomes:

  A(firstN(rows), lastN(cols));

A(seqN(0,n), pesN(last,n));

At least this is symmetric.

Mail converted by MHonArc 2.6.19+