Ensure consistency in use of value/cell names in method in Series. about framian

[dwhjames]

I can add that to #32, or others can open PRs against that branch.

[dwhjames]

There seems to be a design choice in the implementation of filterValues.

def filterValues(p: V => Boolean): Series[K, V] = {
  val b = new SeriesBuilder[K, V]
  b.sizeHint(this.size)
  cfor(0)(_ < index.size, _ + 1) { i =>
    val ix = index.indexAt(i)
    if (column.isValueAt(ix)) {
      val v = column.valueAt(ix)
      if (p(v)) {
        b += (index.keyAt(i), Value(v))
      }
    }
  }
  b.result()
}

def filterValues(p: V => Boolean): Series[K, V] = {
  val b = new SeriesBuilder[K, V]
  b.sizeHint(this.size)
  cfor(0)(_ < index.size, _ + 1) { i =>
    val ix = index.indexAt(i)
    if (column.isValueAt(ix)) {
      val v = column.valueAt(ix)
      if (p(v)) {
        b += (index.keyAt(i), Value(v))
      }
    } else {
      b += (index.keyAt(i), column.nonValueAt(ix))
    }
  }
  b.result()
}

1. treat the series as though it is dense, so filter out all non values as well as the values that don’t satisfy the predicate.
2. treat the series as though it is sparse, so preserve all cells other than the values that don’t satisfy the predicate.

Option 1, does seem somewhat consistent with other methods with ‘value(s)’ in the name, but I don’t know if the same thing should apply here. Maybe we want both? Maybe these should be called filterByKeys, filterByCells, filterByValues, and filterDenseByValues?

[mrvisser]

Unless there are a lot of existing cases where we would want to filter out values while retaining sparse cells, my preference would be to continue to let filterByValues implicitly strip the Series of all non-value cells, similar to series.values. Filtering by values while maintaining sparse cells could be easily implemented with filterByCells.

If there are sufficient cases to filter a series down by its keys, then I'd be inclined to merge filterByKeys and filterByCells together into something like filterEntries:

def filterEntries(p: (K, Cell[V]) => Boolean): Series[K, V]

[mrvisser]

OTOH, if there are not many use-cases to filter a Series while implicitly removing non-value cells, then I'd be in favour of not having filterValues at all, and just sticking with one filter method: filterEntries.

[dwhjames]

I think you’ve convinced me that filterByValues should act as though the series is dense, and that filterEntries is probably also worth having. However, I think we definitely want to keep filterByKeys, filterByCells, and filterByValues. The reason being is that all of these can have more specialized implementations that are more efficient than filterEntries.

One major optimization we can make to all of these filter methods is to take advantage of the fact that our traversal is in index order when building the output series. At the moment, SeriesBuilder is only designed for the general case, so it assumes nothing about the order that key–cell pairs are appended to the builder. And this means that SeriesBuilder produces a Series with an unordered Index (UnorderedIndex). Instead, all of these filter methods should produce series backed by ordered indexes. That has the nice consequence that filtering any series (with an (un)ordered index) will always produce an efficient index.

filterValues can do even better. If the underlying input column is dense, then it never needs to box primitives to retrieve them from the column, and an even more efficient version of SeriesBuilder can be used that never needs to deal with cells and can produce a DenseColumn as the output. So any series filtered with filterValues will get a series with an ordered index and a dense column.

[dwhjames]

I’ve completed this in #32