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 intomainfrom
ff-2352/add-optimality-gap
Jun 6, 2024
Merged

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

Conversation

@schmit
Copy link

@schmit schmit commented Jun 5, 2024


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

0.0,
0.0,
gamma,
flag_key, subject_key, subject_attributes, None, None, 0.0, 0.0, gamma, 0.0

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)

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:

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
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.

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.

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