8000 MOL-2607/ feat: implement random-k selector by emalgorithm · Pull Request #10 · aivant/peppr · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

MOL-2607/ feat: implement random-k selector #10

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

Merged

Conversation

emalgorithm
Copy link
Contributor
@emalgorithm emalgorithm commented Apr 8, 2025

Summary

Implements a new RandomSelector class that selects the best value from k randomly chosen values in the input array. This allows for easier benchmarking of confidence model, since the perfomance obtained with a RandomSelector is the same that would be obtained with a TopSelector by a random confidence model.

Changes

  • Added RandomSelector class to selector.py
  • Added unit test for the new selector in test_selectors.py

Implementation Details

  • RandomSelector randomly selects k indices from the input array
  • Returns either the minimum or maximum value from the selected subset depending on the smaller_is_better parameter
  • Test verifies that the selector, when run multiple times, produces results with an average close to the expected value

Copy link
linear bot commented Apr 8, 2025

@emalgorithm emalgorithm marked this pull request as ready for review April 8, 2025 15:22
@emalgorithm emalgorithm requested a review from padix-key April 8, 2025 15:22
Copy link
Collaborator
@padix-key padix-key left a comment

Choose a reason for hiding this comment

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

Hi, thanks for adding the selector. I added some comments below.

The best value is chosen from *k* randomly chosen predictions.
"""

def __init__(self, k: int) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be good to be able to provide an optional seed to the constructor that would be used to initialize a NumPy Generator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, addressed in ce196e1!

Check that the :class:`RandomSelector`, when ran multiple times, selects approximately the expected value for known
examples.
"""
selector = peppr.RandomSelector(k=5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The random seed could be used in this test to make it deterministic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in af4a29a.

selector.select(values, smaller_is_better=False) for _ in range(20)
]

assert np.isclose(np.mean(selected_values), 5.0, rtol=0.5)
Copy link
Collaborator
@padix-key padix-key Apr 9, 2025

Choose a reason for hiding this comment

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

Currently this test fails, probably because sometimes the mean is randomly not within the tolerance. Using a much larger k (and more values to select from) would solve this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in af4a29a. I was actually computing the wrong expected value.

@padix-key padix-key changed the title MOL-2607/ feat: implement random-k selector in Peppr MOL-2607/ feat: implement random-k selector Apr 9, 2025
@emalgorithm emalgorithm requested a review from padix-key April 9, 2025 08:55
@padix-key
Copy link
Collaborator

Looks good to me now, thanks!

@emalgorithm emalgorithm merged commit c7f0a7a into main Apr 9, 2025
6 checks passed
@emalgorithm emalgorithm deleted the mol-2607-feat-implement-random-k-selector-in-peppr branch April 9, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0