Pass fees to Kelly #507
Pass fees to Kelly #507
Conversation
| answer.p_yes, | ||
| answer.confidence, | ||
| ) | ||
| kelly_bet = get_kelly_bet_full( |
There was a problem hiding this comment.
Open to opinions here 😄
A few lines above, outcome_token_pool = check_not_none(market.outcome_token_pool) is used, which means that get_kelly_bet_simplified was never going to be used.
Also get_kelly_bet_simplified doesn't accept fees which goes against this PR, but then if someones is going to create markets with huge fees, this other PR should solve that #488
There was a problem hiding this comment.
Makes sense, no objections your honour!
WalkthroughThe changes in this pull request focus on modifying the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests_integration/markets/omen/test_kelly.py (1)
109-109: Great addition of fees to the price impact calculation!This change is consistent with the previous one and ensures that fees are considered in price impact calculations. Using
omen_agent_market.feesmaintains consistency and provides realistic fee handling, contributing to more accurate price impact assessments in the tests.For improved code consistency, consider extracting
omen_agent_market.feesto a local variable at the beginning of the function. This would make the code more readable and easier to maintain if you need to use the fees in more places within the function.Here's a suggested improvement:
def assert_price_impact_converges( omen_agent_market: OmenAgentMarket, max_bet_amount: float, p_yes: float, confidence: float, max_price_impact: float, ) -> None: market_fees = omen_agent_market.fees # ... rest of the function ... kelly_bet = get_kelly_bet_full( # ... other parameters ... fees=market_fees, ) # ... rest of the function ... price_impact = kelly.calculate_price_impact_for_bet_amount( # ... other parameters ... fees=market_fees, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- prediction_market_agent_tooling/deploy/betting_strategy.py (3 hunks)
- prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (1 hunks)
- tests_integration/markets/omen/test_kelly.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (6)
prediction_market_agent_tooling/tools/betting_strategies/kelly_criterion.py (1)
65-65: Approve the removal of the default value for thefeesparameter.This change improves the accuracy of the Kelly Criterion calculation by ensuring that fees are always explicitly provided. It aligns well with the PR objective of incorporating fees into the betting strategy benchmark.
To ensure this change is consistently applied, please run the following script to check for any remaining calls to
get_kelly_bet_fullthat might need updating:✅ Verification successful
Error in the previous verification script:
rgdoes not recognize the file typepython.Please run the following corrected script to ensure all calls to
get_kelly_bet_fullinclude thefeesargument:
All calls to
get_kelly_bet_fullinclude the requiredfeesargument.The verification confirms that every invocation of
get_kelly_bet_fullin the codebase now properly includes thefeesparameter, ensuring consistency and correctness in the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to get_kelly_bet_full and verify they include a fees argument # Search for function calls to get_kelly_bet_full rg --type python -g '!*test*' 'get_kelly_bet_full\s*\(' -A 10 # Note: Manually review the output to ensure all calls include a fees argumentLength of output: 94
Script:
#!/bin/bash # Description: Find all calls to get_kelly_bet_full and verify they include a fees argument # Search for function calls to get_kelly_bet_full in Python files, excluding test files rg --type py -g '!*test*' 'get_kelly_bet_full\s*\(' -A 10 # Note: Manually review the output to ensure all calls include a fees argumentLength of output: 3382
tests_integration/markets/omen/test_kelly.py (2)
94-94: Excellent addition of fees to the Kelly bet calculation!This change aligns perfectly with the PR objective of incorporating fees into the Kelly criterion calculations. By using
omen_agent_market.fees, you ensure that the test uses actual market fees, which will provide more realistic and accurate test results. This is a crucial improvement that will help validate the behavior of the betting strategy under real-world conditions.
Line range hint
1-150: Overall, excellent improvements to the Kelly criterion tests!The changes in this file consistently incorporate fees into both the Kelly criterion calculations and price impact assessments. These modifications align perfectly with the PR objectives and significantly enhance the accuracy and realism of the tests.
Key improvements:
- Fees are now considered in
get_kelly_bet_fullcalculations.- Price impact calculations also account for market fees.
These changes will provide more reliable test results that better reflect real-world scenarios, which is crucial for validating the betting strategy's behavior under actual market conditions.
The only suggestion for improvement is a minor code consistency enhancement, which has been detailed in a previous comment.
Great work on improving the test suite!
prediction_market_agent_tooling/deploy/betting_strategy.py (3)
155-165: Consistent use ofget_kelly_bet_fullwith fees parameter.Great job updating the
KellyBettingStrategyto consistently useget_kelly_bet_fullwith thefeesparameter. This ensures that fees are accurately accounted for in the Kelly bet calculation, aligning the strategy with real-world scenarios where fees impact profitability.
224-224: Includingmarket.feesin price impact calculation enhances accuracy.By passing
market.feestocalculate_price_impact_for_bet_amount, the price impact calculations now properly consider fees, improving the precision of the betting strategy and ensuring more realistic trade evaluations.
283-293: UpdatedMaxAccuracyWithKellyScaledBetsStrategyto account for fees in Kelly bet calculation.Updating the
calculate_tradesmethod to useget_kelly_bet_fullwithfees=market.feesensures that fees are considered in the bet sizing for this strategy as well. This change enhances the strategy's accuracy by reflecting the true cost of trades in markets with fees.
I executed the betting strategy benchmark with no fees vs. with fees, comparison is here: https://www.diffchecker.com/bOvQec3S/
TLDR: With fees included in Kelly, profits are lower (which they also are in the reality), but the best strategies remain the same.