Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[aggr-] allow ranking rows by key column #2417

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

midichef
Copy link
Contributor

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
Copy link
Owner

saulpw commented Jun 6, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

saulpw commented Jul 1, 2024

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

Copy link
Owner

@saulpw saulpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

visidata/aggregators.py Outdated Show resolved Hide resolved
visidata/aggregators.py Outdated Show resolved Hide resolved
visidata/aggregators.py Outdated Show resolved Hide resolved
visidata/aggregators.py Outdated Show resolved Hide resolved
@midichef
Copy link
Contributor Author

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
  1. 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 midichef force-pushed the aggr_rank branch 2 times, most recently from 8078fb6 to c6c608e Compare July 29, 2024 04:20
@midichef
Copy link
Contributor Author

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:
    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
Copy link
Contributor Author

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?

Copy link
Owner

@saulpw saulpw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

midichef commented Oct 8, 2024

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:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants