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

[ff-2352] Add optimality gap to bandit logging#50

Merged
schmit merged 6 commits into
mainfrom
ff-2352/add-optimality-gap
Jun 6, 2024
Merged

[ff-2352] Add optimality gap to bandit logging#50
schmit merged 6 commits into
mainfrom
ff-2352/add-optimality-gap

Conversation

@schmit

@schmit schmit commented Jun 5, 2024

Copy link
Copy Markdown

labels: mergeable

Fixes: #ff-2352

Motivation and Context

The optimality gap helps us understand how much the algorithm is trading of exploration and exploitation

Description

  • Add optimality gap
  • Add some tests to test for logging
  • Fix indexing bug for logging

How has this been tested?

  • Updated tests

Comment thread eppo_client/bandit.py
0.0,
0.0,
gamma,
flag_key, subject_key, subject_attributes, None, None, 0.0, 0.0, gamma, 0.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 the formatting change tripped me up at first; I thought some parameters had been re-ordered. No, this just adds 0.0 as the optimality gap (last parameter)

Comment thread eppo_client/bandit.py
return weights

def select_action(self, flag_key, subject_key, action_weights) -> Tuple[int, str]:
def select_action(self, flag_key, subject_key, action_weights) -> str:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Was this function incorrect before the change, or did you prefer having the selected_idx logic outside?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes the index corresponded to the sorted list, rather than the original list, so returning it was useless and caused the bug.

@aarsilv aarsilv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! I assume we'll want an analogous change in the Java SDK as well

@aarsilv aarsilv assigned schmit and unassigned aarsilv and giorgiomartini0 Jun 6, 2024
@schmit schmit merged commit c6ce44d into main Jun 6, 2024
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