Fix aggregation of ratings - #63
Conversation
| stats = { | ||
| "mean": statistics.mean(scores), | ||
| "range": f"({min(scores):.2f}, ({max(scores):.2f})", | ||
| "count": len(ratings), | ||
| } | ||
| summary = f"Option {option_id}: mean={stats['mean']:.2f}, range={stats['range']}, n={stats['count']}" |
There was a problem hiding this comment.
This construction of a dictionary is weird, the dictionary is not actually used anywhere, it's entries are used for string interpolation on the next line. LLM downlift?
There was a problem hiding this comment.
Well, maybe—I lifted it from here in the original triframe.
There was a problem hiding this comment.
I didn’t want a really long f-string but if I’d thought more about it I could have just used a loose collection of variables.
satojk
left a comment
There was a problem hiding this comment.
I left two comments but they're both nitpicks. I think it's worth quickly addressing them + Sami's comment about the dictionary, but looks good overall. Thanks Pip!
| f"Rater made {len(tool_calls)} calls to rate_options, using first ratings only", | ||
| ) | ||
|
|
||
| ratings = parse_ratings(tool_calls[:1], actor_options) |
There was a problem hiding this comment.
No need for parse_ratings to still take a list of tool_calls, rather than just a single tool_call, right? Could change this to tool_calls[0], and simplify parse_ratings a bit.
| if entry.type == "final_ratings": | ||
| return entry | ||
| return None | ||
| if entry.type != "ratings": |
There was a problem hiding this comment.
This introduces the assumption that the most recent ratings are the most recent entries (rather than the most recent ratings being e.g. the second-to-last and third-to-last entries), which isn't an assumption that was there before. Probably this is fine, but I guess we could also add an and last_ratings so that we only break if we have found at least one entry of type ratings.
3ce4fb8 to
8ef910c
Compare
|
I addressed the PR comments and ran an eval set with id |
8ef910c to
240f9ba
Compare
| pytest.param( | ||
| [create_actor_options(), rating_entry, create_actor_options()], | ||
| [rating_entry], | ||
| id="one_rating_not_last_entry", | ||
| ), |
There was a problem hiding this comment.
See my comment here - I think this should have been
| pytest.param( | |
| [create_actor_options(), rating_entry, create_actor_options()], | |
| [rating_entry], | |
| id="one_rating_not_last_entry", | |
| ), | |
| pytest.param( | |
| [create_actor_options(), rating_entry, create_actor_options()], | |
| [], | |
| id="one_rating_not_last_entry", | |
| ), |
| pytest.param( | ||
| [ | ||
| create_actor_options(), | ||
| rating_entry, | ||
| rating_entry, | ||
| create_actor_options(), | ||
| ], | ||
| [rating_entry, rating_entry], | ||
| id="multiple_ratings", | ||
| ), |
There was a problem hiding this comment.
And this should have been
| pytest.param( | |
| [ | |
| create_actor_options(), | |
| rating_entry, | |
| rating_entry, | |
| create_actor_options(), | |
| ], | |
| [rating_entry, rating_entry], | |
| id="multiple_ratings", | |
| ), | |
| pytest.param( | |
| [ | |
| create_actor_options(), | |
| rating_entry, | |
| rating_entry, | |
| create_actor_options(), | |
| ], | |
| [], | |
| id="multiple_ratings_not_last_entries", | |
| ), |
This PR:
rate_optionsmore than once)Closes #58.
Eval set (gpt-4.1 and claude):
pip-triframe-ag-581d4f90bfe7-y6n6je3qawpca7rg