add convert_rank_to_percentile to RankingCalculator #20
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new convert_rank_to_percentile method to the RankingCalculator class that converts user ranks into percentile values using three different statistical methods. Additionally, the PR refactors assertion-based error handling to use explicit RuntimeError exceptions throughout the codebase.
Key changes:
- Added
convert_rank_to_percentilemethod with support for three percentile calculation methods: "original", "hazen", and "weibull" - Changed
num_active_usersfrom a local variable to an instance variable to support the new functionality - Replaced
assert Falsestatements withraise RuntimeErrorfor better error handling - Updated documentation with comprehensive examples and mathematical formulas for percentile calculations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ale_bench/data.py | Added convert_rank_to_percentile method, changed num_active_users to instance variable, replaced assertions with RuntimeError exceptions, added Literal import |
| tests/test_data.py | Added comprehensive test coverage for convert_rank_to_percentile and num_active_users, updated test expectations from AssertionError to RuntimeError |
| docs/rating_and_ranking.md | Updated documentation with detailed examples, mathematical formulas for percentile methods, and usage guidelines for the new functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex Please review this PR |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| method (RankPercentileMapMethod | str): The mode to use for conversion. Defaults to "weibull". | ||
| "original": percentile = 100.0 * rank / num_active_users | ||
| "hazen": percentile = 100.0 * (rank - 0.5) / (num_active_users + 1) | ||
| "weibull": percentile = 100.0 * rank / (num_active_users + 2) |
There was a problem hiding this comment.
The formula should use + 1 instead of + 2 in the denominator for the Weibull method, as the correct formula is rank / (num_active_users + 1).
| elif method == RankPercentileMapMethod.HAZEN: | ||
| return 100.0 * (rank - 0.5) / (self.num_active_users + 1) | ||
| elif method == RankPercentileMapMethod.WEIBULL: | ||
| return 100.0 * rank / (self.num_active_users + 2) |
There was a problem hiding this comment.
The Weibull percentile calculation uses an incorrect denominator. According to the standard Weibull plotting position formula, it should be (num_active_users + 1), not (num_active_users + 2). The current implementation would produce percentiles that don't match the documented formula or standard statistical references.
| return 100.0 * rank / (self.num_active_users + 2) | |
| return 100.0 * rank / (self.num_active_users + 1) |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
No description provided.