[aggr-] allow ranking rows by key column

[midichef]

This PR adds a rank aggregator that returns a list, and a command addcol-rank, which adds a new column with the rank of each row. Ranks are calculated by comparing key columns.

It also fixes a bug in memo-aggregate where long output takes an extremely long time to show up in the statusbar.
For example: seq 1222333 |vd -, then z+ list. After the list is calculated, visidata will get stuck for many seconds showing processing…, because it's very slow to run format() on a long sequence.

I think it's worth having an aggregator for rank, and the need for a simpler solution than the current method has come up before. On the other hand, I know part of Visidata philosophy is that it's not a spreadsheet. How do people feel about having a rank aggregator?

Also, in its current form, the rank aggregator will give errors when comparing key columns with different types across 2 rows:

File "/home/midichef/.local/lib/python3.10/site-packages/visidata/aggregators.py", line 169, in rank
    keys_sorted = sorted(((rowkey, i) for i, rowkey in enumerate(keys)), key=_key_progress(prog))
TypeError: '<' not supported between instances of 'float' and 'list'

What's the standard way to handle sorting mixed types for Visidata?

[saulpw]

What's the standard way to handle sorting mixed types for Visidata?

The standard way is to convert the column into a known type, and then anything that can't be converted (errors and nulls) become TypedWrappers which are sortable with any type. Does that work acceptably here too?

[midichef]

Yes, that seems like it should work. Should the rank aggregator pick the known type, and if so, which one? Or is it the user who should convert the column?

[saulpw]

Since it's not obvious which type to pick, the user can convert the column.

[saulpw]

I love what this is adding, and I think with a few tweaks it would be even more powerful!

[midichef]

There are two kinds of ranking operations people may want.

1. keycol-based rank within sheet: what is the rank of this row, vs. all rows in the sheet, ranking by the value of its key columns? In this example, the key column is keycol, and the current column col is ignored:

keycol	col	keycol_sheetrank
1	10	1
1	20	1

2	60	2
2	50	2
2	30	2

2. column-based rank within group: when grouping the rows by key columns, what is the rank of this row, within its group? The current column determines the rank. In this example, the current column is col:

keycol	col	col_grouprank
1	10	1
1	20	2

2	60	3
2	50	2
2	30	1

What is a good name for these two aggregators? sheetrank and grouprank? Or maybe rank_key and rank_col?
Any suggestions?

[midichef]

Okay, I implemented a command that adds a column and applies an aggregator to rows after grouping them by key columns. It's addcol-aggregate.

To get this to work with list aggregators, I made a new class ListAggregator for aggregators that return lists. Their most common use would be with addcol-aggregate. Right now the only two ListAggregators are list and rank.

I also tried making a sheetrank aggregator, but it's too different from normal aggregators. Normal aggregators apply to a column, but sheetrank is more for the sheet. So I broke it out into a separate command, addcol-sheetrank.

I'm a bit unsure about the new behavior of the list aggregator when used with addcol-aggregate. Right now, if the input column has cells with Exceptions, they show up in the new column. But the error text shows up on the display, it's not hidden behind an error note. !. So I could use guidance on a couple of issues here:

1. Should these Exceptions be passed through by the list aggregator, or should they be translated to null?
2. If they should be passed through the aggregator, how do I make them look/behave like the original cell with an exception?
   The relevant code is here:
   

   visidata/visidata/aggregators.py

   Line 120 in c6c608e

   vals = [ col.getTypedValue(r) for r in row_group ]

   
   To see it in action, vd sample_data/test.jsonl, then addcol-aggregate list. The key1 and key1_list columns ought to look the same, but the fourth cell in key1_list reads Expecting ':' delimiter: line 1 column 34 (char 33) instead of empty.

[midichef]

There is one detail about the grouping in addcol-aggregate. If the key column holds multiple cells with null, all nulls are grouped together as one group of rows. But if the key column holds multiple error cells, each error cell forms its own unique group of 1 row, even if all the errors have the same traceback text. I didn't design this, it's just how it behaved on sorting. Does that error cell treatment sound reasonable?

[saulpw]

Can we move most of this into features/addcol_sheetrank.py? It's a fair amount of code and I'd like to make it self-contained as much as possible.

[midichef]

Okay, I moved the addcol-sheetrank code into a features/ file.

I had to move the RankAggregator class there too, because it relies on functions that moved with addcol-sheetrank. Because the new file is not just for the addcol-sheetrank functionality, I named it features/rank.py instead of features/addcol_sheetrank.py.

One notable consequence of this move is that the new features/rank.py file modifies vd.aggregators outside of aggregators.py:

visidata/visidata/features/rank.py

Line 41 in ec28446

vd.aggregators['rank'] = RankAggregator('rank', anytype, helpstr='list of ranks, when grouping by key columns', listtype=int)

[saulpw]

Do we need a separate listtype? Seems like we could just use the same aggr.type in both cases, and remove this isinstance (which is usually a code smell for me).

[midichef]

The way list aggregators work now, there is a need for two distinct types, type and listtype. type is for the result of the aggregator. For example, this is used by memo-aggregate. That's why type is anytype for ListAggregators. This type would be used whenever we want to hold the entire result (a list) in a cell.

But we also need a separate type for the elements of the list. This is for when the aggregator result goes in a column, like for addcol-aggregate, where each cell holds not the result itself, but an element of the list result.

If I try to get rid of one or the other types, I run into problems. For RankAggregator, if I get rid of the listtype=int switch to type=int, I get an error in the statusbar for z+ rank:
'''
text_rank=int() argument must be a string, a bytes-like object or a real number, not 'list'
'''
But if instead I make RankAggregator use type=anytype, the column added by addcol-aggregate rank does not get the type int.

The need for two types is awkward. And I see your point about isinstance being a code smell. (That is a helpful heuristic, and I'll use it in the future.) It's accurately pointing out strain in the design: most aggregators produce a single value, list aggregators produce a list.

Maybe rank should not be an aggregator. It's unlikely people want a list object holding the ranks. Most people want a column holding the ranks. What if we replace addcol-aggregate+rank with an equivalent command addcol-grouprank (in addition to the existing addcol-sheetrank)? And we would reserve addcol-aggregate for finding group values like sum, mean, median, as you suggested earlier. What do you think?
(I would also consider changing the name addcol-aggregate. Maybe to addcol-group-aggregate.)

[saulpw]

Okay, this one is quite old and quite big at this point! I gave it another pass through, and my remaining questions have to do with aggr.listtype (in review comment), and the vd.aggregate_list function, which seems like it might be an unnecessary and confusing API function. I've asked @anjakefala to look over the behavior too. It'll be nice to finally get this one merged; thanks for your patience on this!

[midichef]

Let me look into why I added a custom stdev function with b41afba and then removed it in ec28446. That's probably an oversight by me.

[anjakefala]

@midichef I think your PR successfully meets your goals. This is such thoughtfully considered work.

Once you have your fnal changes in, I am going to add an update to the guide in this PR, with documentation.

I have one question for my own clarity: what is rank's intended behaviour when there are multiple key columns?

Say

keycol keycol_2 Item
1 10 Pen
1 20 Pencil
2 60 Book
2 50 Pen
2 30 Book

[midichef]

Okay I submitted two minor changes, 20ba354 (which corrects an accidental reversion of a line from a previous commit) and 9ed9f4b (which adds a few errors/warnings as guardrails, since I expect these operations to be difficult for users to understand at first).
Once these are approved, can you squash them into d35738b ?
I would do it myself now, but I want to make it easy to see that I have only made minor changes today.

[anjakefala]

@midichef Yes, this is why I was going to update the docs as part of the merging of this PR. I think it's really hard to understand what these commands are solely from their name and one-line description (which isn't your fault, these are inherently complicated concepts).

But, I will mull over if I have better names at hand....Maybe a good question for the VisiData discord.

[midichef]

I fixed some merge conflicts with the current develop branch, and rebased all previous fixes onto the latest develop. I also did the squashing that I had requested above. These new commits don't change the substance of any code from the previous commits.

[midichef]

Now I've added f5210e5.
When doing within-group rankings via addcol-aggregate rank, the rank comes after ordering the column elements in ascending order. This commit instead uses the sort order on the column. If it's descending, the elements will be ranked in descending order.

I believe it's the behavior users will expect when the column is already sorted in descending order. That's how I realized I wanted this feature. I had a column sorted descending, and ranked it, and was surprised when it was ranked ascending instead.

[midichef]

I still have a little more to add to this PR. Now that the rank operator uses the sort order of the current column, for consistency I'll make addcol-sheetrank also use the sort order of the key columns. And I need to add Progress updates while ranking is underway.

[midichef]

I've added the pending features. This PR is done in terms of implementing features.

What remains to do is to come up with intuitive names and explanations for addcol-aggregate rank vs addcol-sheetrank. I don't have any ideas on how to better do that.

Also, I'm not sure why the ci-build tests are failing, as the failing test tests/load-http.vd passes for me on my system, but is failing here on Github.

[anjakefala]

@midichef It might be a one-off CI blunder. Though, the test output is supposed to show when the error code is 1 😭 I'll have to investigate that.

[midichef]

I've got one more small change queued up to add to this PR: better error handling if the sort fails during ranking. It's a minor change and shouldn't require much review.

[anjakefala]

I changed addcol-sheetrank to addcol-rank-sheet to better fit the naming scheme. =)

I do have a guide I'm working on, but my brain has been a bit cloudy, and I don't want to block this PR on me finishing it. So I'll merge for now, and push the guide when it's ready.

Thanks for all your work @midichef!