Skip to content
This repository was archived by the owner on Nov 8, 2024. It is now read-only.

fix: Use action key to select best scoring action#62

Merged
typotter merged 2 commits intomainfrom
tp/fix/tiebreak
Aug 16, 2024
Merged

fix: Use action key to select best scoring action#62
typotter merged 2 commits intomainfrom
tp/fix/tiebreak

Conversation

@typotter
Copy link
Collaborator

🎟️ Fixes: FF-3060
👯 Universal Test PR: sdk-test-data/52

Motivation and Context

Tie-breaker by action name was introduced to the bandit evaluation algorithm to eliminate ambiguity when multiple actions are the top scoring

Description

  • Selects top scoring action by score then by action key

How has this been tested?

Universal tests

@typotter typotter requested review from aarsilv and schmit August 15, 2024 19:20
best_action = max(action_scores, key=action_scores.get)
best_score = action_scores[best_action]
# Find the max score
best_score = max(action_scores.values())
Copy link

Choose a reason for hiding this comment

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

Damnit so close to keeping it a one line, but this takes the largest lexicographically ordered key

Suggested change
best_score = max(action_scores.values())
max(d, key=lambda x: (d.get(x), x))

:(

Copy link

@schmit schmit left a comment

Choose a reason for hiding this comment

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

Clean fix!

@schmit
Copy link

schmit commented Aug 15, 2024

Just mak esure to make the linter and formatter happy before merging :)

Copy link
Contributor

@aarsilv aarsilv left a comment

Choose a reason for hiding this comment

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

👍
I'm ok with two lines 😅

@typotter typotter merged commit 2c51eea into main Aug 16, 2024
@typotter typotter deleted the tp/fix/tiebreak branch August 16, 2024 02:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants