Add max_price_impact parameter to KellyBettingStrategy
#433
Conversation
WalkthroughThe pull request introduces a comprehensive suite of unit tests for the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
Results from this simulation look promising - already better than
Note that Next steps |
|
Some updated simulations Highlights:
|
| else new_yes / (new_yes + new_no) | ||
| ) | ||
| # print(f"actual_price {actual_price} actual_price2 {actual_price_2}") | ||
| s = (actual_price - expected_price) / expected_price |
There was a problem hiding this comment.
The sign of s is the opposite to what you'd get from the formula in this comment. Does it make a difference?
There was a problem hiding this comment.
-> If actual_price > expected_price, you paid more for the same amount of tokens, hence slippage should be > 0.
-> If actual_price < 'expected_price`, you paid less, hence slippage < 0.
So I guess the formula is correct per the above, or would you disagree?
There was a problem hiding this comment.
Also note that
-> By the way, one correction ref slippage - in the "normal case" (see example here https://docs.gnosis.io/conditionaltokens/docs/introduction3/#an-example-with-cpmm), slippage is >0 (new_price_yes = 0.8, old_price = 0.5, hence positive slippage) (edited)
-> Now, in the example above, let's say you place a bet NO with the same 10 xDAI. If you do the math, you derive that you got 23 .33 NO tokens (invariant becomes 15 Y * 6.67 No = 100), but you expected 20 tokens (since original price was 0.2). Hence this is a <0 slippage (you paid cheaper than expected)
12:32
So slippage can be negative!
There was a problem hiding this comment.
Okay, this makes sense now. And it also makes sense to me that this works as expected when called in slippage_diff 😄. Lots of not very intuitive things for me to get my head around
| try: | ||
| optimized_bet_amount = minimize_scalar( | ||
| slippage_diff, | ||
| bounds=(min(yes_outcome_pool_size, no_outcome_pool_size) / 10, 100), |
There was a problem hiding this comment.
If self.max_slippage is very small, then the root might be smaller than the lower bound. Similarly, if the pool is very large, the root could be bigger than 100. Is [0, inf] okay?
There was a problem hiding this comment.
-> 0 is not OK since get_buy_outcome_token_amount breaks for 0
if ending_outcome_balance <= 0:
raise ValueError("must have non-zero balances")
For that reason I used a small size as bound, which might be too large already.
-> inf works, I can try with sys.maxsize or similar
There was a problem hiding this comment.
@evangriffiths please take a look the updated versions now, should address the concerns above.
I don't see any more logs with deviating values. I also changed the log to an Error, so it's obvious when values are wrong.
…track-experiments-kelly # Conflicts: # examples/monitor/match_bets_with_langfuse_traces.py # poetry.lock
| ] | ||
|
|
||
| storage = hishel.FileStorage(ttl=3600) | ||
| controller = hishel.Controller(force_cache=True) | ||
| httpx_client = hishel.CacheClient(storage=storage, controller=controller) |
There was a problem hiding this comment.
That's interesting, what about cutting it out to some helper function like get_cached_httpx_client?
There was a problem hiding this comment.
Abstracted it away (see class HttpxCachedClient)
| @@ -1,5 +1,10 @@ | |||
| from abc import ABC, abstractmethod | |||
|
|
|||
| import numpy as np | |||
| from loguru import logger | |||
There was a problem hiding this comment.
| from loguru import logger | |
| from prediction_market_agent_tooling.loggers import logger |
| slippage, self.max_price_impact, self.max_price_impact / 100 | ||
| ): | ||
| logger.info( | ||
| f"Slippage {slippage} deviates too much from self.max_slippage {self.max_price_impact}, market_id {market.id}" |
There was a problem hiding this comment.
deviates too much
It sounds pretty bad, yet it's logged as info, and the code continues. Can it be just ignored?
There was a problem hiding this comment.
I see this happen a lot if I run the script on this branch. If this is ever hit, does that mean there's a bug in calculate_bet_amount_for_price_impact?
There was a problem hiding this comment.
Indeed the minimize_scalar was complaining - I adjusted bounds and now it looks better. Please give it a go and let me know if you find issues!
Please note that this is now a ValueError since I want to enforce these boundaries.
| # We just don't want Kelly size to extrapolate price_impact - hence we take the min. | ||
| kelly_bet_size = min(kelly_bet.size, max_slippage_bet_amount) | ||
|
|
||
| self._check_price_impact_ok_else_log( |
There was a problem hiding this comment.
So it's possible it's not ok, but it just continues?
There was a problem hiding this comment.
Please check revised version - now it's a ValueError.
| def calculate_bet_amount_for_price_impact( | ||
| self, market: AgentMarket, kelly_bet: SimpleBet, fee: float | ||
| ) -> float: | ||
| def calculate_price_impact_deviation_from_target_price_impact(b: xDai) -> float: |
There was a problem hiding this comment.
Why does this need to be a function within the function?
There was a problem hiding this comment.
It doesn't have to be, it just felt simpler to have this here since it's not needed anywhere else in the class.
There was a problem hiding this comment.
Hmm, that's almost never done in Python. Can you just make it a normal function, please?
There was a problem hiding this comment.
I dunno, I think it's fine in this context, especially as it's being used as a Callable, passed as an arg to minimize_scalar
ChatGPT agrees:
in python is it good practice to define a function inside a function?
...
If the inner function is being passed as a callable to another function, this can be a valid and clean design choice, especially if the callable depends on the state of the outer function
There was a problem hiding this comment.
Also I did the same thing here
😄There was a problem hiding this comment.
Okay then, I'm standing overvoted 😄 For sure it's not a blocker.
scripts/calc_slippage.py
Outdated
| @@ -0,0 +1,18 @@ | |||
| r_a = 10 | |||
There was a problem hiding this comment.
In PMAT, all scripts have been helpful in executing some actions so far.
This feels more for the examples folder. 🤔
tests/markets/omen/test_kelly.py
Outdated
| kelly: KellyBettingStrategy, | ||
| ): | ||
| # expect | ||
| expected_price = yes / (yes + no) # p_yes |
There was a problem hiding this comment.
Isn't this p_no?
There was a problem hiding this comment.
I changed yes, no = no, yes if buy_direction is False.
Tests taken from https://docs.gnosis.io/conditionaltokens/docs/introduction3/#an-example-with-cpmm pass.
kongzii
dismissed
their stale review
Sorry, accidentaly clicked on Approve while submitting
Can you post these for all agents we have available? No need for the full table, just What I'm wondering is if this strategy is more profitable on all of them. To check if this isn't an over-fit for this particular agent. |
|
Some more results Notes:
DeployablePredictionProphetGPT4TurboFinalAgent23 bets
DeployablePredictionProphetGPT4TurboPreviewAgent23 bets
DeployableOlasEmbeddingOAAgent23 bets
DeployableKnownOutcomeAgent23 bets
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
prediction_market_agent_tooling/tools/httpx_cached_client.py (2)
1-3: Consider specifying exact imports and removing extra empty line.While the current import is functional, it's generally a good practice to specify the exact components being imported from a library. This can improve code readability and potentially reduce import time. Also, there's an extra empty line that can be removed.
Consider updating the imports as follows:
-import hishel - +from hishel import FileStorage, Controller, CacheClient
10-11: Add a docstring and consider makingclienta public attribute.The
get_clientmethod is simple, but its purpose isn't immediately clear. Adding a docstring would improve clarity. Additionally, if there's no specific reason for using a getter method (e.g., lazy initialization or additional logic), consider making theclientattribute public.Here's a suggested improvement:
- def get_client(self): - return self.client + def get_client(self): + """ + Returns the cached HTTP client instance. + + Returns: + hishel.CacheClient: The cached HTTP client. + """ + return self.clientAlternatively, if no additional logic is needed:
class HttpxCachedClient: def __init__(self, ttl=3600, check_ttl_every=600): storage = hishel.FileStorage(ttl=ttl, check_ttl_every=check_ttl_every) controller = hishel.Controller(force_cache=True) - self.client = hishel.CacheClient(storage=storage, controller=controller) - - def get_client(self): - return self.client + self.client = hishel.CacheClient(storage=storage, controller=controller)tests/markets/omen/test_kelly.py (4)
6-13: LGTM: Well-structured test case with room for improvement.The test case is well-structured and based on a documented example. It correctly sets up the KellyBettingStrategy and the scenario for testing price impact.
Consider adding assertions for the expected output or behavior directly in this test function, in addition to using the helper function. This would make the test's purpose more immediately clear to readers.
16-23: LGTM: Good follow-up test case with potential for enhancement.This test case provides a different scenario from the first test, which is excellent for increasing test coverage. It correctly simulates a state after a bet has been placed.
Consider the following enhancements:
- Add assertions for the expected output directly in this test function, similar to the suggestion for the first test.
- Consider parameterizing these tests to cover more scenarios easily.
- Add a brief explanation of how this scenario differs from the first one and what specific aspect of the strategy it's testing.
26-50: LGTM: Well-implemented helper function with minor improvement suggestions.The
assert_price_impactfunction correctly implements the price impact calculation and handles different buy directions. The use ofnp.isclosefor floating-point comparison is appropriate.Consider the following improvements:
- Remove the print statement at line 50 or wrap it in a debug condition, as it's unnecessary for automated tests and may clutter the output.
- Add type hints for the return value of the function (-> None).
- Consider raising a custom exception with a descriptive message instead of using the default AssertionError. This would provide more context when a test fails.
Example implementation for point 3:
if not np.isclose(price_impact, expected_price_impact, rtol=0.01): raise ValueError(f"Price impact mismatch: expected {expected_price_impact}, got {price_impact}")
1-50: Enhance overall test coverage and structure.The current test file provides a good foundation for testing the KellyBettingStrategy, but there's room for improvement in terms of coverage and structure.
Consider the following enhancements:
- Implement parameterized tests to cover a wider range of scenarios with less code duplication.
- Add edge case tests (e.g., very small or large bet amounts, extreme yes/no ratios).
- Include tests for error conditions and invalid inputs.
- Consider separating the helper function into a fixture or a separate utility module if it's likely to be used in other test files.
- Add a test class to group related tests and potentially share setup/teardown logic.
Example of a parameterized test using pytest:
import pytest @pytest.mark.parametrize("yes,no,bet_amount,buy_direction,expected_impact", [ (10, 10, 10, True, 0.5), (5, 20, 10, False, 0.4), # Add more test cases here ]) def test_kelly_slippage_calculation(yes, no, bet_amount, buy_direction, expected_impact): kelly = KellyBettingStrategy(max_bet_amount=1, max_price_impact=0.5) price_impact = kelly.calculate_price_impact_for_bet_amount( buy_direction, bet_amount=bet_amount, yes=yes, no=no, fee=0 ) assert np.isclose(price_impact, expected_impact, rtol=0.01)This approach would make it easier to add more test cases and improve overall coverage.
prediction_market_agent_tooling/deploy/betting_strategy.py (4)
145-148: Validatemax_price_impactin the constructorIn the
__init__method,max_price_impactis accepted without validation. Consider adding validation to ensure that if provided,max_price_impactis a positive float. This will help catch configuration errors early.Apply this diff to add validation:
def __init__(self, max_bet_amount: float, max_price_impact: float | None = None): self.max_bet_amount = max_bet_amount + if max_price_impact is not None and max_price_impact <= 0: + raise ValueError("max_price_impact must be a positive float or None") self.max_price_impact = max_price_impact
200-201: Use market fee instead of hardcoded zeroWhen calling
calculate_bet_amount_for_price_impact, thefeeparameter is hardcoded as0. To accurately reflect transaction costs, consider using thefeeattribute from themarketobject if available.Apply this diff to use the actual market fee:
max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( - market, kelly_bet, 0 + market, kelly_bet, market.fee if hasattr(market, 'fee') else 0 )
161-166: Adjust tolerance in price impact comparisonThe comparison between
price_impactandself.max_price_impactuses an absolute tolerance (atol). For values that can vary proportionally, a relative tolerance (rtol) might be more appropriate to prevent false positives when values are very small.Consider changing
np.iscloseto usertol:if price_impact > self.max_price_impact and not np.isclose( - price_impact, self.max_price_impact, atol=self.max_price_impact * 0.01 + price_impact, self.max_price_impact, rtol=0.01 ): raise ValueError( f"Price impact {price_impact} deviates too much from self.max_price_impact {self.max_price_impact}, market_id {market.id}" )
224-236: Clarify the calculation ofprice_impactThe calculation of
price_impactmight benefit from additional comments to enhance readability and understanding, especially regarding the expectations of positive or negative slippage.Consider adding explanatory comments:
actual_price = bet_amount / tokens_to_buy +# A positive price impact indicates the actual price is higher than expected (slippage > 0) +# A negative price impact indicates the actual price is lower than expected (slippage < 0) price_impact = (actual_price - expected_price) / expected_price
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (4)
- examples/monitor/match_bets_with_langfuse_traces.py (6 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (5 hunks)
- prediction_market_agent_tooling/tools/httpx_cached_client.py (1 hunks)
- tests/markets/omen/test_kelly.py (1 hunks)
🧰 Additional context used
🪛 Ruff
examples/monitor/match_bets_with_langfuse_traces.py
300-300: f-string without any placeholders
Remove extraneous
fprefix(F541)
prediction_market_agent_tooling/deploy/betting_strategy.py
239-239: Undefined name
self(F821)
240-240: Undefined name
kelly_bet_direction(F821)
241-241: Undefined name
bet_amount(F821)
242-242: Undefined name
yes_outcome_pool_size(F821)
243-243: Undefined name
no_outcome_pool_size(F821)
244-244: Undefined name
fee(F821)
247-247: Undefined name
self(F821)
🔇 Additional comments (12)
tests/markets/omen/test_kelly.py (1)
1-3: LGTM: Appropriate imports for the test file.The import statements are correct and necessary for the tests being performed. numpy is used for floating-point comparison, and KellyBettingStrategy is the class under test.
examples/monitor/match_bets_with_langfuse_traces.py (10)
12-13: ✔️ Appropriate Import of Necessary ClassesThe addition of
ProbabilisticAnswerandTradeTypeimports is appropriate as they are utilized within theget_outcome_for_tracefunction.
20-20: ✔️ Correct Import ofHttpxCachedClientImporting
HttpxCachedClientallows for efficient HTTP request caching, which enhances performance when interacting with external APIs.
37-40: ✔️ Proper Definition ofMSEProfitClassThe
MSEProfitclass is well-defined with appropriate type annotations, encapsulating lists for mean squared error (p_yes_mse) and total profit (total_profit).
102-102: ✔️ Secure Retrieval of Private KeysThe
agent_pkey_mapis correctly initialized by retrieving private keys from GCP Secret Manager, ensuring secure handling of sensitive information.
114-146: ✔️ Enhanced Strategy Testing with Varying ParametersIntroducing multiple instances of
KellyBettingStrategywith varyingmax_bet_amountandmax_price_impactparameters enriches the strategy simulations, allowing for comprehensive analysis under different conditions.
149-150: ✔️ Initialization of Cached HTTP ClientThe
httpx_clientis properly initialized usingHttpxCachedClient, which will cache HTTP responses and improve the efficiency of network operations.
169-169: ✔️ Passing Cached HTTP Client to LangfuseProviding the
httpx_clientto the Langfuse client ensures that all HTTP requests benefit from caching, improving overall performance.
240-242: ✔️ Exporting Detailed Simulation DataSaving the detailed CSV files for each agent and strategy combination facilitates deeper analysis of individual bets and their outcomes.
285-285: Ensure Validp_yes mseValues in Simulations DataWhen adding
p_yes mseto your simulations data, confirm that it is a valid number to avoid issues in data analysis and reporting.Please verify that
p_yes_mseis valid before adding it:- "p_yes mse": p_yes_mse, + "p_yes mse": p_yes_mse if not math.isnan(p_yes_mse) else None,
301-305: Handle Cases with Insufficient Data for CorrelationWhen calculating correlation, ensure that there are at least two data points. If the lists
mseandprofithave fewer than two items,pandaswill returnnan.Consider adding a check:
if len(mse) > 1 and len(profit) > 1: correlation = pd.Series(mse).corr(pd.Series(profit)) else: correlation = float('nan') # or handle appropriatelyprediction_market_agent_tooling/deploy/betting_strategy.py (1)
272-278: 🛠️ Refactor suggestionReview optimization bounds and iteration limits
In
minimize_scalar, the bounds are set to(min(yes_outcome_pool_size, no_outcome_pool_size) / 1000, 1000), andmaxiteris set to 10,000. Ensure that the lower bound is appropriate and that the number of iterations is sufficient without being excessive. Extremely high iteration counts can impact performance.Please verify that the optimization converges efficiently with these parameters and adjust if necessary. Consider using dynamic bounds based on market conditions.
| def __init__(self): | ||
| storage = hishel.FileStorage(ttl=3600, check_ttl_every=600) | ||
| controller = hishel.Controller(force_cache=True) | ||
| self.client = hishel.CacheClient(storage=storage, controller=controller) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making TTL and check interval configurable, and explain force_cache=True.
The current implementation uses hardcoded values for TTL and check interval. To improve flexibility, consider making these configurable through constructor parameters with default values. Additionally, it would be helpful to add a comment explaining the implications of setting force_cache=True.
Here's a suggested refactor:
class HttpxCachedClient:
- def __init__(self):
- storage = hishel.FileStorage(ttl=3600, check_ttl_every=600)
- controller = hishel.Controller(force_cache=True)
+ def __init__(self, ttl=3600, check_ttl_every=600):
+ storage = hishel.FileStorage(ttl=ttl, check_ttl_every=check_ttl_every)
+ # force_cache=True ensures that cached responses are always used when available,
+ # potentially improving performance but may serve stale data
+ controller = hishel.Controller(force_cache=True)
self.client = hishel.CacheClient(storage=storage, controller=controller)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__(self): | |
| storage = hishel.FileStorage(ttl=3600, check_ttl_every=600) | |
| controller = hishel.Controller(force_cache=True) | |
| self.client = hishel.CacheClient(storage=storage, controller=controller) | |
| class HttpxCachedClient: | |
| def __init__(self, ttl=3600, check_ttl_every=600): | |
| storage = hishel.FileStorage(ttl=ttl, check_ttl_every=check_ttl_every) | |
| # force_cache=True ensures that cached responses are always used when available, | |
| # potentially improving performance but may serve stale data | |
| controller = hishel.Controller(force_cache=True) | |
| self.client = hishel.CacheClient(storage=storage, controller=controller) |
| def calculate_price_impact_deviation_from_target_price_impact(b: xDai) -> float: | ||
| price_impact = self.calculate_price_impact_for_bet_amount( | ||
| kelly_bet_direction, | ||
| bet_amount, | ||
| yes_outcome_pool_size, | ||
| no_outcome_pool_size, | ||
| fee, | ||
| ) | ||
| # We return abs for the algorithm to converge to 0 instead of the min (and possibly negative) value. | ||
| return abs(price_impact - self.max_price_impact) |
There was a problem hiding this comment.
Remove duplicate method and fix undefined variables
The method calculate_price_impact_deviation_from_target_price_impact at lines 238-247 appears to be a duplicate and references undefined variables (self, kelly_bet_direction, bet_amount, yes_outcome_pool_size, no_outcome_pool_size, fee). This method is already correctly defined as an inner function within calculate_bet_amount_for_price_impact. Removing this duplicate will prevent potential errors and confusion.
Apply this diff to remove the duplicate method:
- def calculate_price_impact_deviation_from_target_price_impact(b: xDai) -> float:
- price_impact = self.calculate_price_impact_for_bet_amount(
- kelly_bet_direction,
- bet_amount,
- yes_outcome_pool_size,
- no_outcome_pool_size,
- fee,
- )
- # We return abs for the algorithm to converge to 0 instead of the min (and possibly negative) value.
- return abs(price_impact - self.max_price_impact)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def calculate_price_impact_deviation_from_target_price_impact(b: xDai) -> float: | |
| price_impact = self.calculate_price_impact_for_bet_amount( | |
| kelly_bet_direction, | |
| bet_amount, | |
| yes_outcome_pool_size, | |
| no_outcome_pool_size, | |
| fee, | |
| ) | |
| # We return abs for the algorithm to converge to 0 instead of the min (and possibly negative) value. | |
| return abs(price_impact - self.max_price_impact) |
🧰 Tools
🪛 Ruff
239-239: Undefined name
self(F821)
240-240: Undefined name
kelly_bet_direction(F821)
241-241: Undefined name
bet_amount(F821)
242-242: Undefined name
yes_outcome_pool_size(F821)
243-243: Undefined name
no_outcome_pool_size(F821)
244-244: Undefined name
fee(F821)
247-247: Undefined name
self(F821)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (12)
tests/markets/omen/test_kelly.py (4)
6-13: Consider adding documentation for KellyBettingStrategy parameters.The test case is well-structured and based on a documented example. However, it would be beneficial to add comments explaining the significance of the
max_bet_amount=1andmax_price_impact=0.5parameters for the KellyBettingStrategy. This would help readers understand how these parameters influence the test scenario.
16-23: Clarify the relationship between test cases.The comment suggests that this test case represents the state after the first bet, but it's not immediately clear how this relates to
test_kelly_slippage_calculation1. Consider adding more detailed comments to explain:
- How the values of
yesandnowere derived.- Why the
bet_amountis still 10 if this is a follow-up to the first bet.- The significance of changing
buy_directiontoFalse.This additional context would make the test cases more understandable and maintainable.
50-50: Remove print statement in assert_price_impact function.The print statement at the end of the
assert_price_impactfunction is likely used for debugging purposes. Consider removing it or wrapping it in a debug flag check to keep the test output clean in normal runs.- print(price_impact) + # Uncomment for debugging + # print(price_impact)
41-45: Add comments explaining the price impact calculation formula.The price impact calculation in the
assert_price_impactfunction uses a specific formula that may not be immediately clear to all readers. Consider adding comments to explain:
- The meaning of the
kconstant.- The derivation of the
tokens_bought_yesformula.- How
actual_price_yesandexpected_price_impactare calculated.This would improve the readability and maintainability of the test code.
examples/monitor/match_bets_with_langfuse_traces.py (4)
149-156: Improved HTTP handling and strategy performance trackingThe changes introduce two important improvements:
The use of
HttpxCachedClientcould enhance performance by caching HTTP requests, potentially reducing network load and response times.The
strat_mse_profitsdictionary provides a structured way to store and analyze Mean Squared Error (MSE) and profit data for each strategy.These additions will facilitate more efficient data retrieval and comprehensive strategy analysis.
Suggestion for improvement:
Consider using a more robust identifier for strategy keys in thestrat_mse_profitsdictionary. The current use ofrepr(strategy)might lead to issues if the__repr__method of the strategy classes changes. A more stable approach could be to implement aunique_idmethod in the strategy classes and use that for dictionary keys.Here's a suggested implementation:
class BettingStrategy: def unique_id(self): return f"{self.__class__.__name__}_{id(self)}" # Then use: strat_mse_profits[strategy.unique_id()] = MSEProfit(p_yes_mse=[], total_profit=[])This approach ensures unique and stable identifiers for each strategy instance.
240-242: Enhanced data export for detailed analysisThe addition of CSV exports for each agent-strategy combination is a valuable improvement:
- It allows for persistent storage of detailed simulation results.
- The exported data facilitates post-processing and in-depth analysis of strategy performance.
This change will greatly enhance the ability to perform offline analysis and compare strategies across different scenarios.
Suggestion for improvement:
Consider adding a timestamp or unique identifier to the CSV filename to prevent overwriting previous results and to facilitate tracking of multiple simulation runs.Here's a suggested implementation:
import time # ... timestamp = int(time.time()) pd.DataFrame.from_records(details).to_csv( f"{agent_name} - {strategy} - all bets - {timestamp}.csv", index=False )This ensures that each simulation run produces a uniquely identifiable output file.
Line range hint
244-305: Improved strategy evaluation with MSE and correlation analysisThe addition of Mean Squared Error (MSE) calculation and correlation analysis between MSE and total profit is a significant improvement:
- MSE calculation provides a quantitative measure of prediction accuracy for each strategy.
- The correlation analysis helps understand the relationship between prediction accuracy and profitability.
These additions will provide valuable insights into strategy performance and the importance of accurate predictions.
Suggestions for improvement:
- Handle potential division by zero in MSE calculation:
if len(bets_with_traces) > 0: p_yes_mse = sum_squared_errors / len(bets_with_traces) else: p_yes_mse = float('inf') # or another appropriate value
- Consider visualizing the correlation results:
import matplotlib.pyplot as plt plt.figure(figsize=(10, 6)) for strategy_name, mse_profit in strat_mse_profits.items(): plt.scatter(mse_profit.p_yes_mse, mse_profit.total_profit, label=strategy_name) plt.xlabel('MSE') plt.ylabel('Total Profit') plt.legend() plt.title('MSE vs Total Profit for Different Strategies') plt.savefig('mse_vs_profit.png')This will create a scatter plot showing the relationship between MSE and total profit for all strategies, providing a visual representation of the correlation.
🧰 Tools
🪛 Ruff
300-300: f-string without any placeholders
Remove extraneous
fprefix(F541)
300-300: Simplify print statementThe print statement on line 300 uses an f-string without any placeholders. This is unnecessary and can be simplified.
Apply this change to simplify the print statement:
- print(f"Correlation between p_yes mse and total profit:") + print("Correlation between p_yes mse and total profit:")This change removes the unnecessary f-string prefix, making the code slightly cleaner.
🧰 Tools
🪛 Ruff
300-300: f-string without any placeholders
Remove extraneous
fprefix(F541)
prediction_market_agent_tooling/deploy/betting_strategy.py (4)
3-5: Ensure compliance with import order conventionsThe newly added imports should follow the project's import order conventions. Group and order the imports appropriately (e.g., standard libraries, third-party libraries, local modules) for better readability and maintainability.
153-153: Clarify the docstring to accurately describe the method's purposeThe docstring
"Bet size cannot be larger than max_price_impact"does not accurately reflect the method's functionality. Since the method asserts that the price impact is less than or equal tomax_price_impact, consider rephrasing the docstring for clarity.Apply this diff to update the docstring:
-"""Bet size cannot be larger than max_price_impact""" +"""Assert that price impact is less than or equal to max_price_impact"""
250-251: Use a more appropriate exception typeRaising an
EnvironmentErrorwhenmax_price_impactis not set might not be the best choice, especially sinceEnvironmentErroris deprecated in Python 3. Consider usingValueErrorto indicate that an invalid value was provided.Apply this diff to update the exception:
-if not self.max_price_impact: - raise EnvironmentError("This method requires max_price_impact to be set") +if self.max_price_impact is None: + raise ValueError("max_price_impact must be set to use this method")
282-286: Review the optimization bounds inminimize_scalarThe bounds
(min(yes_outcome_pool_size, no_outcome_pool_size) / 1000, 1000)might not be suitable for all market conditions. If the outcome pool sizes are small, the lower bound could be too close to zero, potentially causing convergence issues.Consider adjusting the bounds dynamically based on market conditions or adding safeguards to prevent optimization failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- examples/monitor/match_bets_with_langfuse_traces.py (6 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (5 hunks)
- prediction_market_agent_tooling/tools/httpx_cached_client.py (1 hunks)
- tests/markets/omen/test_kelly.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/tools/httpx_cached_client.py
🧰 Additional context used
🪛 Ruff
examples/monitor/match_bets_with_langfuse_traces.py
300-300: f-string without any placeholders
Remove extraneous
fprefix(F541)
🔇 Additional comments (9)
tests/markets/omen/test_kelly.py (3)
1-3: LGTM: Appropriate imports for the test file.The import statements are correct and necessary for the tests being conducted. numpy is used for floating-point comparison, and KellyBettingStrategy is the class being tested.
38-39: Confirmation: Implementation of past comment suggestion.The code correctly implements the suggestion from a past comment to swap
yesandnovalues whenbuy_directionis False. This implementation aligns with the example from the Gnosis documentation and ensures correct calculations for both buy directions.
1-50: Summary: Well-structured tests with room for improved documentation.Overall, the test file for KellyBettingStrategy is well-structured and covers important scenarios for price impact calculations. The tests seem to align with the Gnosis documentation example and handle different buy directions correctly.
To further improve the file:
- Add more detailed comments explaining the KellyBettingStrategy parameters and the relationship between test cases.
- Document the price impact calculation formula for better understanding.
- Consider removing or conditionally enabling the debug print statement.
These changes will enhance the readability and maintainability of the tests, making it easier for other developers to understand and extend the test suite in the future.
examples/monitor/match_bets_with_langfuse_traces.py (4)
20-20: Improved HTTP handling and new data structure for analysisThe addition of
HttpxCachedClientand the newMSEProfitclass are positive changes:
HttpxCachedClientcould potentially improve performance by caching HTTP requests.MSEProfitprovides a structured way to store and analyze Mean Squared Error (MSE) and profit data.These additions enhance the code's capabilities for performance optimization and data analysis.
Also applies to: 37-40
114-146: Expanded strategy testing with fine-tuned parametersThe addition of multiple new strategies, especially the variations of
KellyBettingStrategywith differentmax_bet_amountandmax_price_impactvalues, is a significant improvement:
- It allows for more comprehensive testing and comparison of betting strategies.
- The fine-tuned parameters enable a deeper understanding of how these factors affect strategy performance.
This expansion will provide valuable insights into optimal betting strategies under various conditions.
Consider the following next steps:
- Analyze the performance of these strategies to identify the most effective combinations of
max_bet_amountandmax_price_impact.- Consider creating a heatmap or surface plot to visualize the performance across different parameter combinations.
- Use the insights gained to potentially create adaptive strategies that adjust these parameters based on market conditions.
169-169: Enhanced Langfuse client initializationThe addition of
httpx_clientto the Langfuse constructor is a positive change:
- It allows the Langfuse client to utilize the cached HTTP client, potentially improving performance by reducing redundant network requests.
- This change is consistent with the earlier introduction of
HttpxCachedClient, maintaining a cohesive approach to HTTP request handling throughout the script.This modification should lead to more efficient API interactions with Langfuse.
Line range hint
1-305: Overall assessment: Significant improvements in strategy testing and analysisThe changes made to this file represent a substantial enhancement in the functionality and analysis capabilities of the script:
- Expanded strategy testing with fine-tuned parameters allows for more comprehensive evaluation of betting strategies.
- Improved HTTP handling with
HttpxCachedClientpotentially optimizes performance.- Introduction of MSE calculation and correlation analysis provides deeper insights into strategy performance.
- Enhanced data export facilitates detailed offline analysis.
These improvements will greatly aid in identifying optimal betting strategies and understanding the relationship between prediction accuracy and profitability.
Recommended next steps:
- Implement the suggested improvements for code robustness and data persistence.
- Add visualization of correlation results to aid in interpretation.
- Consider running extensive simulations with the new strategies to identify optimal parameter combinations.
- Use the insights gained from these analyses to potentially develop adaptive strategies that can adjust parameters based on market conditions.
prediction_market_agent_tooling/deploy/betting_strategy.py (2)
290-290: Update__repr__method to reflect new parametersIncluding
max_price_impactin the__repr__method ensures a complete representation of the object's state. This change improves debugging and logging clarity.
170-175: Verify the appropriateness of the tolerance innp.iscloseThe comparison using
np.isclosehas an absolute tolerance (atol) of1%ofself.max_price_impact. Ensure that this tolerance is suitable for your application's requirements and doesn't inadvertently allow unacceptable price impact deviations.Run the following script to evaluate the effect of different tolerance values:
This script will help determine if the current tolerance meets the desired precision.
| logger.warning( | ||
| f"Could not assert price impact, check variables: market.outcome_token_pool " | ||
| f"{market.outcome_token_pool} and self.max_price_impact {self.max_price_impact}" | ||
| ) | ||
| return |
There was a problem hiding this comment.
Handle missing values with exceptions instead of warnings
When market.outcome_token_pool or self.max_price_impact is not set, the method logs a warning and returns, potentially bypassing critical checks. Consider raising an exception to prevent unintended behavior due to missing required values.
Apply this diff to raise exceptions for missing values:
-if not market.outcome_token_pool or not self.max_price_impact:
- logger.warning(
- f"Could not assert price impact, check variables: market.outcome_token_pool "
- f"{market.outcome_token_pool} and self.max_price_impact {self.max_price_impact}"
- )
- return
+if not market.outcome_token_pool:
+ raise ValueError("Market outcome_token_pool is None")
+if self.max_price_impact is None:
+ raise ValueError("max_price_impact is not set")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.warning( | |
| f"Could not assert price impact, check variables: market.outcome_token_pool " | |
| f"{market.outcome_token_pool} and self.max_price_impact {self.max_price_impact}" | |
| ) | |
| return | |
| if not market.outcome_token_pool: | |
| raise ValueError("Market outcome_token_pool is None") | |
| if self.max_price_impact is None: | |
| raise ValueError("max_price_impact is not set") |
| max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( | ||
| market, kelly_bet, 0 | ||
| ) |
There was a problem hiding this comment.
Include market fees in price impact calculations
The fee parameter is currently set to 0 when calling calculate_bet_amount_for_price_impact. If the market has a non-zero fee, this could lead to inaccurate calculations. Pass the actual market fee to ensure precise price impact estimation.
Apply this diff to include the market fee:
-max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact(
- market, kelly_bet, 0
+max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact(
+ market, kelly_bet, fee=market.fee if hasattr(market, 'fee') else 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( | |
| market, kelly_bet, 0 | |
| ) | |
| max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( | |
| market, kelly_bet, fee=market.fee if hasattr(market, 'fee') else 0 | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
tests/markets/omen/test_kelly.py (4)
6-13: LGTM: Well-structured test based on Gnosis documentation.The test case is clear and based on official documentation, which is excellent for maintaining consistency with expected behavior. The use of the
assert_price_impacthelper function promotes code reusability.Consider adding a comment explaining the expected outcome of this test case. This would make the test's purpose more explicit and easier to understand at a glance.
16-23: LGTM: Good test case for a different scenario.This test case effectively covers a different scenario with unequal yes and no values, and a False buy_direction. The comment explaining the context is helpful.
To improve clarity, consider adding an assertion or comment about the expected outcome of this test, similar to the suggestion for the previous test function.
26-50: LGTM: Well-implemented helper function with room for minor improvements.The
assert_price_impactfunction is well-structured and correctly handles both buy and sell scenarios. The calculations for price impact seem to follow the expected logic, and the use ofnp.isclosefor floating-point comparison is appropriate.Consider the following improvements:
- Remove the print statement on line 50 or wrap it in a debug condition, as it's generally unnecessary for automated tests.
- Add type hints for the return value of the function (-> None).
- Consider adding more detailed comments explaining the price impact calculation, especially for the more complex formulas.
To improve code clarity and maintainability, consider extracting the price impact calculation logic into a separate function. This would make the
assert_price_impactfunction more focused on assertion and easier to read. For example:def calculate_expected_price_impact(yes: float, no: float, bet_amount: float) -> float: expected_price_yes = no / (yes + no) k = yes * no tokens_bought_yes = (yes + bet_amount) - (k / (no + bet_amount)) actual_price_yes = bet_amount / tokens_bought_yes return (actual_price_yes - expected_price_yes) / expected_price_yes def assert_price_impact(...) -> None: # ... existing code ... expected_price_impact = calculate_expected_price_impact(yes, no, bet_amount) # ... rest of the function ...
1-50: Consider expanding the test suite for more comprehensive coverage.The current test suite provides a good foundation with two distinct test cases and a reusable helper function. To further improve the robustness of the tests, consider the following suggestions:
Add more test cases to cover a wider range of scenarios, such as:
- Edge cases (e.g., very small or very large bet amounts)
- Different maximum bet amounts and maximum price impacts
- Cases where the price impact exceeds the maximum allowed
- Scenarios with non-zero fees
Implement parametrized tests using pytest.mark.parametrize to run the same test logic with different inputs, reducing code duplication.
Add tests for any error conditions or edge cases in the KellyBettingStrategy class that aren't currently covered.
Consider adding integration tests that verify the interaction between KellyBettingStrategy and other components of the system, if applicable.
To improve the overall structure and maintainability of the test suite, consider organizing the tests into a class structure. This can help group related tests and share common setup code. For example:
import pytest from prediction_market_agent_tooling.deploy.betting_strategy import KellyBettingStrategy class TestKellyBettingStrategy: @pytest.fixture def kelly_strategy(self): return KellyBettingStrategy(max_bet_amount=1, max_price_impact=0.5) def test_kelly_slippage_calculation1(self, kelly_strategy): # Test logic here def test_kelly_slippage_calculation2(self, kelly_strategy): # Test logic here @pytest.mark.parametrize("yes,no,bet_amount,buy_direction,expected_impact", [ (10, 10, 10, True, 0.5), (5, 20, 10, False, 0.4), # Add more test cases here ]) def test_price_impact(self, kelly_strategy, yes, no, bet_amount, buy_direction, expected_impact): # Parametrized test logic hereThis structure allows for easier expansion of the test suite and better organization of test cases.
examples/monitor/match_bets_with_langfuse_traces.py (4)
114-146: LGTM: Expanded strategy testing with varied parametersThe addition of new strategies, especially the KellyBettingStrategy with various
max_bet_amountandmax_price_impactparameters, allows for more comprehensive testing. This should provide valuable insights into strategy performance under different market conditions.Consider creating a function to generate these strategies programmatically. This could make it easier to add or modify strategies in the future and improve code maintainability. For example:
def generate_kelly_strategies(max_bet_amounts, max_price_impacts): return [ KellyBettingStrategy(max_bet_amount=amount, max_price_impact=impact) for amount in max_bet_amounts for impact in max_price_impacts ] # Usage kelly_strategies = generate_kelly_strategies( max_bet_amounts=[2, 5, 25], max_price_impacts=[0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7] ) strategies.extend(kelly_strategies)
153-156: LGTM: Enhanced strategy performance analysis with MSE trackingThe addition of MSE calculation and tracking for each strategy is a valuable enhancement. It provides a metric for prediction accuracy and allows for more detailed analysis of strategy performance.
Consider extracting the MSE calculation into a separate function for better readability and reusability. For example:
def calculate_mse(bets_with_traces): sum_squared_errors = sum( (trace.answer.p_yes - float(bet.market_outcome)) ** 2 for bet_with_trace in bets_with_traces for bet, trace in [(bet_with_trace.bet, bet_with_trace.trace)] ) return sum_squared_errors / len(bets_with_traces) if bets_with_traces else 0This would simplify the main loop and make the code more maintainable.
Also applies to: 244-251, 274-277
300-305: LGTM: Added correlation analysis between MSE and total profitThe addition of correlation analysis between MSE and total profit for each strategy is a valuable feature. This can provide insights into the relationship between prediction accuracy and profitability.
Consider using an f-string for the print statement on line 300 to maintain consistency with the rest of the code:
- print(f"Correlation between p_yes mse and total profit:") + print("Correlation between p_yes mse and total profit:")🧰 Tools
🪛 Ruff
300-300: f-string without any placeholders
Remove extraneous
fprefix(F541)
Line range hint
1-305: Overall: Significant improvements to betting strategy evaluation and analysisThis update introduces several valuable enhancements to the betting strategy evaluation process:
- Addition of
HttpxCachedClientfor optimized API calls- Introduction of MSE calculation for prediction accuracy assessment
- Expanded strategy testing with varied parameters
- Enhanced data export and reporting capabilities
- Correlation analysis between MSE and total profit
These changes collectively provide a more comprehensive framework for analyzing and comparing different betting strategies. The improvements in data collection, analysis, and reporting will facilitate better decision-making and strategy refinement.
As the complexity of this script grows, consider breaking it down into smaller, more focused modules. This could include separate modules for strategy generation, data collection, analysis, and reporting. This modular approach would improve maintainability and make it easier to extend functionality in the future.
prediction_market_agent_tooling/deploy/betting_strategy.py (4)
3-5: New imports added for price impact calculationsThe new imports (
numpy,logger, andminimize_scalar) are appropriate for the added functionality. However, consider the following suggestions:
- Import
loggerfrom a centralized logging module instead of directly fromlogurufor better maintainability.- Consider using
from scipy.optimize import minimize_scalarinstead of importing the entirescipy.optimizemodule for better performance.
146-149: Updated__init__method withmax_price_impactparameterThe addition of the
max_price_impactparameter with a default value ofNoneis a good approach. It allows for backward compatibility while introducing the new functionality. However, consider adding type hints for better code readability and maintainability.Consider updating the method signature as follows:
def __init__(self, max_bet_amount: float, max_price_impact: float | None = None) -> None:
150-179: Improveassert_price_impact_lt_max_price_impact_else_raisemethodThe method correctly checks if the price impact is within the specified limit. However, consider the following improvements:
- Add a more descriptive docstring explaining the purpose and parameters of the method.
- The use of
np.isclosewith a relative tolerance is good, but consider making the tolerance a configurable parameter.- Improve the error message to include the actual price impact and the maximum allowed impact.
Here's a suggested improvement for the error message:
raise ValueError( f"Price impact {price_impact:.4f} exceeds the maximum allowed impact of {max_price_impact:.4f} for market {market.id}" )
234-249: Improvecalculate_price_impact_for_bet_amountmethodThe price impact calculation looks correct, but consider the following improvements:
- Add a descriptive docstring explaining the purpose, parameters, and return value of the method.
- Add type hints to improve code readability and maintainability.
- Consider handling potential division by zero when calculating
expected_priceandprice_impact.Here's a suggested improvement with type hints and a docstring:
def calculate_price_impact_for_bet_amount( self, buy_direction: bool, bet_amount: float, yes: float, no: float, fee: float ) -> float: """ Calculate the price impact for a given bet amount. Args: buy_direction (bool): True if buying Yes tokens, False if buying No tokens. bet_amount (float): The amount of the bet. yes (float): The current balance of Yes tokens in the pool. no (float): The current balance of No tokens in the pool. fee (float): The fee percentage. Returns: float: The calculated price impact as a percentage. Raises: ValueError: If the total outcome tokens or expected price is zero. """ total_outcome_tokens = yes + no if total_outcome_tokens == 0: raise ValueError("Total outcome tokens cannot be zero") expected_price = (no if buy_direction else yes) / total_outcome_tokens if expected_price == 0: raise ValueError("Expected price cannot be zero") tokens_to_buy = get_buy_outcome_token_amount(bet_amount, buy_direction, yes, no, fee) actual_price = bet_amount / tokens_to_buy price_impact = (actual_price - expected_price) / expected_price return price_impact
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- examples/monitor/match_bets_with_langfuse_traces.py (6 hunks)
- prediction_market_agent_tooling/deploy/betting_strategy.py (5 hunks)
- prediction_market_agent_tooling/tools/httpx_cached_client.py (1 hunks)
- tests/markets/omen/test_kelly.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/tools/httpx_cached_client.py
🧰 Additional context used
🪛 Ruff
examples/monitor/match_bets_with_langfuse_traces.py
300-300: f-string without any placeholders
Remove extraneous
fprefix(F541)
🔇 Additional comments (7)
tests/markets/omen/test_kelly.py (1)
1-3: LGTM: Imports are appropriate for the tests.The imports of
numpyandKellyBettingStrategyare relevant and necessary for the tests being performed in this file.examples/monitor/match_bets_with_langfuse_traces.py (4)
20-20: LGTM: Improved HTTP request handling with cachingThe addition of
HttpxCachedClientis a good optimization. It should help reduce API call latency and improve overall performance, especially when dealing with repeated requests.
37-40: LGTM: Well-structured data model for MSE and profit trackingThe
MSEProfitclass is a good addition for organizing MSE and profit data. Using Pydantic'sBaseModelensures type safety and facilitates easy data handling.
149-150: LGTM: Implemented caching for HTTP requestsThe use of
HttpxCachedClientfor the Langfuse client is a good optimization. This should reduce redundant API calls and improve overall performance, especially when dealing with repeated requests to the same endpoints.Also applies to: 169-169
240-242: LGTM: Improved data export and reporting capabilitiesThe addition of detailed CSV exports and the inclusion of MSE values in the markdown report are excellent enhancements. These changes will greatly facilitate in-depth analysis of strategy performance and enable better visualization of results.
Also applies to: 291-298
prediction_market_agent_tooling/deploy/betting_strategy.py (2)
295-295: LGTM: Updated__repr__methodThe
__repr__method has been correctly updated to include themax_price_impactparameter. This change provides a more complete representation of the object's state.
Line range hint
1-359: Overall review summary and recommendationsThe changes to incorporate price impact calculations into the
KellyBettingStrategyclass are well-implemented and add valuable functionality to the betting strategy. Here are the key points and recommendations:
The new methods for price impact calculations (
calculate_price_impact_for_bet_amountandcalculate_bet_amount_for_price_impact) are logically sound and use appropriate numerical optimization techniques.The
calculate_tradesmethod has been successfully modified to adjust bet sizes based on price impact considerations.To further improve the code, consider the following recommendations:
- Add comprehensive docstrings to all new methods, explaining their purpose, parameters, and return values.
- Implement consistent type hinting throughout the class to improve code readability and maintainability.
- Enhance error handling, particularly for edge cases like division by zero in price impact calculations.
- Consider extracting some of the price impact logic into a separate class or module to maintain single responsibility principle.
- Make certain parameters (e.g., optimization bounds, tolerance levels) configurable for more flexibility.
The changes are backwards compatible due to the optional
max_price_impactparameter, which is a good design choice.Overall, the implementation is solid and achieves the goal of incorporating price impact into the Kelly betting strategy. With the suggested improvements, the code will be more robust, maintainable, and easier to understand for future developers.
| kelly_bet_size = kelly_bet.size | ||
| if self.max_price_impact: | ||
| # Adjust amount | ||
| max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( | ||
| market, kelly_bet, 0, self.max_price_impact | ||
| ) | ||
|
|
||
| # We just don't want Kelly size to extrapolate price_impact - hence we take the min. | ||
| kelly_bet_size = min(kelly_bet.size, max_slippage_bet_amount) | ||
|
|
||
| self.assert_price_impact_lt_max_price_impact_else_raise( | ||
| kelly_bet.direction, kelly_bet_size, market, self.max_price_impact | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve clarity and modularity in calculate_trades method
The changes to incorporate price impact considerations are good, but consider the following improvements:
- Add comments to explain the logic behind taking the minimum of
kelly_bet.sizeandmax_slippage_bet_amount. - Extract the bet size adjustment logic into a separate method for better modularity and readability.
- Consider handling the case when
self.max_price_impactis None more explicitly.
Consider refactoring the bet size adjustment logic as follows:
def adjust_bet_size_for_price_impact(self, kelly_bet: SimpleBet, market: AgentMarket) -> float:
if not self.max_price_impact:
return kelly_bet.size
max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact(
market, kelly_bet, 0, self.max_price_impact
)
# Take the minimum to ensure Kelly size doesn't exceed the price impact limit
adjusted_bet_size = min(kelly_bet.size, max_slippage_bet_amount)
self.assert_price_impact_lt_max_price_impact_else_raise(
kelly_bet.direction, adjusted_bet_size, market, self.max_price_impact
)
return adjusted_bet_size
# In calculate_trades method:
kelly_bet_size = self.adjust_bet_size_for_price_impact(kelly_bet, market)| def calculate_bet_amount_for_price_impact( | ||
| self, | ||
| market: AgentMarket, | ||
| kelly_bet: SimpleBet, | ||
| fee: float, | ||
| max_price_impact: float, | ||
| ) -> float: | ||
| def calculate_price_impact_deviation_from_target_price_impact( | ||
| bet_amount: xDai, | ||
| ) -> float: | ||
| price_impact = self.calculate_price_impact_for_bet_amount( | ||
| kelly_bet.direction, | ||
| bet_amount, | ||
| yes_outcome_pool_size, | ||
| no_outcome_pool_size, | ||
| fee, | ||
| ) | ||
| # We return abs for the algorithm to converge to 0 instead of the min (and possibly negative) value. | ||
|
|
||
| return abs(price_impact - max_price_impact) | ||
|
|
||
| if not market.outcome_token_pool: | ||
| logger.warning( | ||
| "Market outcome_token_pool is None, cannot calculate bet amount" | ||
| ) | ||
| return kelly_bet.size | ||
|
|
||
| yes_outcome_pool_size = market.outcome_token_pool[ | ||
| market.get_outcome_str_from_bool(True) | ||
| ] | ||
| no_outcome_pool_size = market.outcome_token_pool[ | ||
| market.get_outcome_str_from_bool(False) | ||
| ] | ||
|
|
||
| optimized_bet_amount = minimize_scalar( | ||
| calculate_price_impact_deviation_from_target_price_impact, | ||
| bounds=(min(yes_outcome_pool_size, no_outcome_pool_size) / 1000, 1000), | ||
| method="bounded", | ||
| tol=1e-11, | ||
| options={"maxiter": 10000}, | ||
| ) | ||
| return float(optimized_bet_amount.x) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve calculate_bet_amount_for_price_impact method
The optimization approach using minimize_scalar is good. However, consider the following improvements:
- Add a descriptive docstring explaining the purpose, parameters, and return value of the method.
- Add type hints to improve code readability and maintainability.
- Consider making the optimization bounds more flexible or configurable.
- The inner function
calculate_price_impact_deviation_from_target_price_impactcould be moved outside the method for better readability.
Here's a suggested improvement with type hints and a docstring:
from typing import Callable
def calculate_price_impact_deviation_from_target_price_impact(
self,
bet_amount: float,
kelly_bet: SimpleBet,
yes_outcome_pool_size: float,
no_outcome_pool_size: float,
fee: float,
max_price_impact: float
) -> float:
price_impact = self.calculate_price_impact_for_bet_amount(
kelly_bet.direction,
bet_amount,
yes_outcome_pool_size,
no_outcome_pool_size,
fee,
)
return abs(price_impact - max_price_impact)
def calculate_bet_amount_for_price_impact(
self,
market: AgentMarket,
kelly_bet: SimpleBet,
fee: float,
max_price_impact: float,
) -> float:
"""
Calculate the optimal bet amount for a given price impact using numerical optimization.
Args:
market (AgentMarket): The market to calculate the bet amount for.
kelly_bet (SimpleBet): The Kelly bet object containing direction and size.
fee (float): The fee percentage.
max_price_impact (float): The maximum allowed price impact.
Returns:
float: The optimal bet amount that achieves the target price impact.
"""
if not market.outcome_token_pool:
logger.warning("Market outcome_token_pool is None, cannot calculate bet amount")
return kelly_bet.size
yes_outcome_pool_size = market.outcome_token_pool[market.get_outcome_str_from_bool(True)]
no_outcome_pool_size = market.outcome_token_pool[market.get_outcome_str_from_bool(False)]
optimization_func: Callable[[float], float] = lambda bet_amount: self.calculate_price_impact_deviation_from_target_price_impact(
bet_amount, kelly_bet, yes_outcome_pool_size, no_outcome_pool_size, fee, max_price_impact
)
lower_bound = min(yes_outcome_pool_size, no_outcome_pool_size) / 1000
upper_bound = 1000 # Consider making this configurable
optimized_bet_amount = minimize_scalar(
optimization_func,
bounds=(lower_bound, upper_bound),
method="bounded",
tol=1e-11,
options={"maxiter": 10000},
)
return float(optimized_bet_amount.x)…track-experiments-kelly
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
prediction_market_agent_tooling/deploy/betting_strategy.py (3)
150-174: New method:assert_price_impact_lt_max_price_impact_else_raiseThis method ensures that the price impact of a bet doesn't exceed the specified maximum. The implementation looks correct, but there are a few points to consider:
- The method name is quite long. Consider a shorter name like
assert_price_impact_within_limit.- The use of
np.isclosewith a relative tolerance is a good approach to handle floating-point comparisons.- The error message in the
ValueErroris informative, which is good for debugging.Consider renaming the method to improve readability:
def assert_price_impact_within_limit( self, buy_direction: bool, bet_size: float, market: AgentMarket, ) -> None: # ... rest of the method remains the same
204-216: Updatedcalculate_tradesmethod with price impact considerationThe modifications to include price impact calculations in the
calculate_tradesmethod are well-implemented. However, there are a few points to consider:
- The comment "Adjust amount" could be more descriptive.
- The hardcoded
0passed tocalculate_bet_amount_for_price_impactfor thefeeparameter might not be correct for all markets.Consider updating the comment and using the market's fee:
# Calculate bet amount considering max price impact max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( market, kelly_bet, market.fee )
229-244: New method:calculate_price_impact_for_bet_amountThis method calculates the price impact based on the bet amount and market conditions. The implementation looks correct, but there's a minor issue with a comment:
The comment "price_impact should always be > 0" is not always true. Price impact can be negative in certain market conditions. Consider updating the comment to reflect this:
# Note: price_impact can be positive or negative depending on market conditions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py (5 hunks)
🔇 Additional comments (5)
prediction_market_agent_tooling/deploy/betting_strategy.py (5)
3-5: New imports added for price impact calculationsThe additions of
numpy,logger, andminimize_scalarare appropriate for the new price impact calculation functionality. These libraries will be useful for numerical operations, logging, and optimization respectively.
18-20: Import ofget_buy_outcome_token_amountfunctionThe import of
get_buy_outcome_token_amountfrom the Omen module is now explicitly stated. This is good for clarity and helps in understanding the dependencies of the price impact calculations.
146-148: Updated constructor withmax_price_impactparameterThe addition of the
max_price_impactparameter with a default value ofNoneis a good approach. It allows for backwards compatibility while introducing the new price impact functionality.
290-290: Updated__repr__methodThe
__repr__method has been updated to include themax_price_impactparameter. This is good for debugging and logging purposes.
Line range hint
1-290: Overall assessment of changesThe modifications to the
KellyBettingStrategyclass significantly enhance its functionality by incorporating price impact considerations. The new methods for calculating and asserting price impact are well-implemented, and the updates to thecalculate_tradesmethod effectively integrate these new features.There are a few minor suggestions for improvement:
- Consider renaming the
assert_price_impact_lt_max_price_impact_else_raisemethod for better readability.- Ensure that market fees are correctly used in price impact calculations.
- Update comments to accurately reflect the behavior of price impact calculations.
These changes provide a more sophisticated betting strategy that should perform better in real-world market conditions by considering the impact of bets on market prices.
| def calculate_bet_amount_for_price_impact( | ||
| self, | ||
| market: AgentMarket, | ||
| kelly_bet: SimpleBet, | ||
| fee: float, | ||
| ) -> float: | ||
| def calculate_price_impact_deviation_from_target_price_impact( | ||
| bet_amount: xDai, | ||
| ) -> float: | ||
| price_impact = self.calculate_price_impact_for_bet_amount( | ||
| kelly_bet.direction, | ||
| bet_amount, | ||
| yes_outcome_pool_size, | ||
| no_outcome_pool_size, | ||
| fee, | ||
| ) | ||
| # We return abs for the algorithm to converge to 0 instead of the min (and possibly negative) value. | ||
|
|
||
| max_price_impact = check_not_none(self.max_price_impact) | ||
| return abs(price_impact - max_price_impact) | ||
|
|
||
| if not market.outcome_token_pool: | ||
| logger.warning( | ||
| "Market outcome_token_pool is None, cannot calculate bet amount" | ||
| ) | ||
| return kelly_bet.size | ||
|
|
||
| yes_outcome_pool_size = market.outcome_token_pool[ | ||
| market.get_outcome_str_from_bool(True) | ||
| ] | ||
| no_outcome_pool_size = market.outcome_token_pool[ | ||
| market.get_outcome_str_from_bool(False) | ||
| ] | ||
|
|
||
| optimized_bet_amount = minimize_scalar( | ||
| calculate_price_impact_deviation_from_target_price_impact, | ||
| bounds=(min(yes_outcome_pool_size, no_outcome_pool_size) / 1000, 1000), | ||
| method="bounded", | ||
| tol=1e-11, | ||
| options={"maxiter": 10000}, | ||
| ) | ||
| return float(optimized_bet_amount.x) |
There was a problem hiding this comment.
New method: calculate_bet_amount_for_price_impact
This method uses numerical optimization to find the bet amount that achieves the target price impact. The implementation is generally good, but there are a few points to consider:
- The use of
minimize_scalarwith bounded optimization is appropriate. - The inner function
calculate_price_impact_deviation_from_target_price_impactis well-structured. - The error handling for missing
outcome_token_poolis good.
However, there's a potential issue:
The fee parameter is passed to this method but not used in the price impact calculation. This could lead to inaccurate results if the market has a non-zero fee. Consider updating the inner function to use the fee parameter:
price_impact = self.calculate_price_impact_for_bet_amount(
kelly_bet.direction,
bet_amount,
yes_outcome_pool_size,
no_outcome_pool_size,
fee, # Use the fee parameter here
)
| price_impact = self.calculate_price_impact_for_bet_amount( | ||
| buy_direction, | ||
| bet_size, | ||
| outcome_token_pool["Yes"], |
There was a problem hiding this comment.
Somewhere down the codes you used
market.outcome_token_pool[market.get_outcome_str_from_bool(True)]
and
market.outcome_token_pool[market.get_outcome_str_from_bool(False)]
That's more future-proof for no extra costs
There was a problem hiding this comment.
Good point - implemented.
tests/markets/omen/test_kelly.py
Outdated
| @@ -0,0 +1,50 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
These test are in the omen directory but they don't have anything specific about Omen
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
tests/test_kelly.py (3)
6-13: Enhance test coverage and readability.While the test case is based on Gnosis documentation, consider the following improvements:
- Add assertions within the function body to verify the initial state of the
KellyBettingStrategyinstance.- Include comments explaining the expected behavior or outcome of the test.
- Consider parameterizing the test to cover multiple scenarios easily.
Here's a suggested improvement:
def test_kelly_slippage_calculation1() -> None: # Test case based on https://docs.gnosis.io/conditionaltokens/docs/introduction3/#an-example-with-cpmm kelly = KellyBettingStrategy(max_bet_amount=1, max_price_impact=0.5) # Verify initial state assert kelly.max_bet_amount == 1 assert kelly.max_price_impact == 0.5 # Equal probability scenario yes = 10 no = 10 bet_amount = 10 buy_direction = True # Expected outcome: Price impact should be calculated correctly for a buy direction in an equal probability scenario assert_price_impact(bet_amount, buy_direction, yes, no, kelly)
16-24: Enhance test coverage and readability.Similar to the first test case, consider the following improvements:
- Add assertions within the function body to verify the initial state of the
KellyBettingStrategyinstance.- Include comments explaining the expected behavior or outcome of the test.
- Consider parameterizing the test to cover multiple scenarios easily.
Here's a suggested improvement:
def test_kelly_slippage_calculation2() -> None: # Test case based on https://docs.gnosis.io/conditionaltokens/docs/introduction3/#an-example-with-cpmm kelly = KellyBettingStrategy(max_bet_amount=1, max_price_impact=0.5) # Verify initial state assert kelly.max_bet_amount == 1 assert kelly.max_price_impact == 0.5 # Scenario after a bet of 10 xDAI on Yes yes = 5 no = 20 bet_amount = 10 buy_direction = False # Expected outcome: Price impact should be calculated correctly for a sell direction after a previous bet assert_price_impact(bet_amount, buy_direction, yes, no, kelly)
1-51: Overall good start, but room for improvement in test coverage and structure.The file provides a good foundation for testing the
KellyBettingStrategyclass, particularly focusing on price impact calculations. However, there are several areas where the tests could be enhanced:
- Increase test coverage by adding more test cases with different scenarios.
- Consider using parameterized tests to easily cover multiple input combinations.
- Add more assertions within each test function to verify the state of the
KellyBettingStrategyinstance and intermediate calculations.- Improve code readability by adding more comments explaining the expected behavior and outcomes of each test.
- Consider organizing the tests into a test class, which could help with setup and teardown of common test fixtures.
To improve the overall structure and maintainability of the tests, consider the following:
- Create a
TestKellyBettingStrategyclass to group related tests.- Use
pytest.fixturefor common setup code.- Implement parameterized tests using
pytest.mark.parametrizefor testing multiple scenarios efficiently.Example structure:
import pytest from prediction_market_agent_tooling.deploy.betting_strategy import KellyBettingStrategy class TestKellyBettingStrategy: @pytest.fixture def kelly_strategy(self): return KellyBettingStrategy(max_bet_amount=1, max_price_impact=0.5) @pytest.mark.parametrize("yes,no,bet_amount,buy_direction,expected_impact", [ (10, 10, 10, True, ...), # Fill in expected impact (5, 20, 10, False, ...), # Fill in expected impact # Add more test cases here ]) def test_price_impact_calculation(self, kelly_strategy, yes, no, bet_amount, buy_direction, expected_impact): price_impact = kelly_strategy.calculate_price_impact_for_bet_amount( buy_direction, bet_amount=bet_amount, yes=yes, no=no, fee=0 ) assert pytest.approx(price_impact, rel=1e-2) == expected_impact # Add more test methods as neededThis structure would make it easier to add and maintain tests as the
KellyBettingStrategyclass evolves.prediction_market_agent_tooling/deploy/betting_strategy.py (4)
146-148: New parameter added for price impact control.The addition of
max_price_impactparameter enhances the flexibility of theKellyBettingStrategy. This is a good improvement that allows for more fine-grained control over the betting behavior.Consider using
Optional[float]instead offloat | Nonefor better compatibility with Python versions below 3.10:from typing import Optional def __init__(self, max_bet_amount: float, max_price_impact: Optional[float] = None):
150-174: New method to assert price impact is within limits.This method provides a crucial check to ensure that the price impact of a bet doesn't exceed the specified maximum. The use of
np.isclosefor floating-point comparison is a good practice.Consider improving the error handling by providing more context in the error message:
raise ValueError( f"Price impact {price_impact:.4f} exceeds max_price_impact {max_price_impact:.4f} " f"by {(price_impact - max_price_impact) / max_price_impact:.2%}. " f"Market ID: {market.id}" )This change provides more detailed information about the extent of the violation, which could be helpful for debugging and monitoring.
229-244: New method to calculate price impact.This method provides a crucial calculation for determining the price impact of a bet. The logic seems correct and uses appropriate market functions.
However, the comment "price_impact should always be > 0" might not be accurate in all cases. Price impact can be negative if the actual price is lower than the expected price. Consider updating the comment to:
# Note: price_impact can be positive or negative depending on market conditionsThis change acknowledges that price impact can vary based on market dynamics.
246-287: New method for optimizing bet amount based on price impact.This method effectively uses numerical optimization to find the optimal bet amount that achieves the target price impact. The use of
minimize_scalaris appropriate for this task.Consider the following improvements:
- Add a docstring to explain the purpose and parameters of the method.
- Handle the case when
self.max_price_impactis None more explicitly.- Consider making the optimization bounds more flexible or configurable.
Here's a suggested improvement:
def calculate_bet_amount_for_price_impact( self, market: AgentMarket, kelly_bet: SimpleBet, fee: float, ) -> float: """ Calculate the optimal bet amount for a given price impact using numerical optimization. Args: market (AgentMarket): The market to calculate the bet amount for. kelly_bet (SimpleBet): The Kelly bet object containing direction and size. fee (float): The fee percentage. Returns: float: The optimal bet amount that achieves the target price impact. """ if self.max_price_impact is None: return kelly_bet.size if not market.outcome_token_pool: logger.warning("Market outcome_token_pool is None, cannot calculate bet amount") return kelly_bet.size yes_outcome_pool_size = market.outcome_token_pool[market.get_outcome_str_from_bool(True)] no_outcome_pool_size = market.outcome_token_pool[market.get_outcome_str_from_bool(False)] lower_bound = min(yes_outcome_pool_size, no_outcome_pool_size) / 1000 upper_bound = max(1000, kelly_bet.size * 2) # Adjust upper bound based on Kelly bet size # ... rest of the method remains the sameThese changes improve the method's clarity, error handling, and flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- prediction_market_agent_tooling/deploy/betting_strategy.py (5 hunks)
- tests/test_kelly.py (1 hunks)
🔇 Additional comments (3)
tests/test_kelly.py (1)
1-3: LGTM: Imports are appropriate for the tests.The import statements are correct and necessary for the tests being performed.
numpyis used for floating-point comparison, andKellyBettingStrategyis the class being tested.prediction_market_agent_tooling/deploy/betting_strategy.py (2)
3-5: New imports added for enhanced functionality.The additions of
numpy,logger, andminimize_scalarfrom scipy suggest new mathematical operations and optimization techniques are being introduced. This is a good practice for expanding the capabilities of the betting strategy.
289-290: Updated string representation to include max_price_impact.The
__repr__method has been correctly updated to include themax_price_impactparameter. This change ensures that the string representation of the class accurately reflects its current state, including the new parameter.
tests/test_kelly.py
Outdated
| def assert_price_impact( | ||
| bet_amount: float, | ||
| buy_direction: bool, | ||
| yes: float, | ||
| no: float, | ||
| kelly: KellyBettingStrategy, | ||
| ) -> None: | ||
| price_impact = kelly.calculate_price_impact_for_bet_amount( | ||
| buy_direction, bet_amount=bet_amount, yes=yes, no=no, fee=0 | ||
| ) | ||
|
|
||
| # Calculation is done assuming buy_direction is True. Else, we invert the reserves. | ||
| if not buy_direction: | ||
| yes, no = no, yes | ||
|
|
||
| expected_price_yes = no / (yes + no) | ||
| k = yes * no | ||
| tokens_bought_yes = (yes + bet_amount) - (k / (no + bet_amount)) # 23.333 # x*y = k | ||
| actual_price_yes = bet_amount / tokens_bought_yes | ||
| expected_price_impact = (actual_price_yes - expected_price_yes) / expected_price_yes | ||
| #### | ||
|
|
||
| assert np.isclose(price_impact, expected_price_impact, rtol=0.01) | ||
|
|
||
| print(price_impact) |
There was a problem hiding this comment.
💡 Codebase verification
Unnecessary print statements found in test files.
The following print statements are present:
tests/test_kelly.py: print(price_impact)tests/markets/omen/test_omen.py: print(position) # For extra test coverage
Consider removing these print statements or replacing them with proper logging to avoid cluttering test outputs.
🔗 Analysis chain
Minor improvements and clarification needed.
The assert_price_impact function looks well-implemented overall. However, consider the following suggestions:
- Add type hints for the return value of
calculate_price_impact_for_bet_amount. - Consider using constants for magic numbers like 0.01 in the np.isclose call.
- The print statement at the end might not be necessary for automated tests. If it's for debugging, consider using a logger or removing it.
Here's a suggested improvement:
import logging
from typing import Any
RELATIVE_TOLERANCE = 0.01
def assert_price_impact(
bet_amount: float,
buy_direction: bool,
yes: float,
no: float,
kelly: KellyBettingStrategy,
) -> None:
price_impact: float = kelly.calculate_price_impact_for_bet_amount(
buy_direction, bet_amount=bet_amount, yes=yes, no=no, fee=0
)
# ... (rest of the function remains the same)
assert np.isclose(price_impact, expected_price_impact, rtol=RELATIVE_TOLERANCE)
logging.debug(f"Price impact: {price_impact}")Could you clarify the purpose of the print statement at the end of the function? Is it intended for debugging or some other purpose?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other print statements in test files
rg --type python 'print\(' tests/
Length of output: 66
Script:
#!/bin/bash
# Search for other print statements in Python test files
rg --type py 'print\(' tests/
Length of output: 154
| kelly_bet_size = kelly_bet.size | ||
| if self.max_price_impact: | ||
| # Adjust amount | ||
| max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( | ||
| market, kelly_bet, 0 | ||
| ) | ||
|
|
||
| # We just don't want Kelly size to extrapolate price_impact - hence we take the min. | ||
| kelly_bet_size = min(kelly_bet.size, max_slippage_bet_amount) | ||
|
|
||
| self.assert_price_impact_lt_max_price_impact_else_raise( | ||
| kelly_bet.direction, kelly_bet_size, market | ||
| ) | ||
|
|
There was a problem hiding this comment.
Bet size adjustment based on price impact.
The addition of price impact consideration in the bet size calculation is a significant improvement. It ensures that the Kelly bet size doesn't exceed the maximum allowed price impact.
However, the hardcoded value of 0 for the fee parameter in calculate_bet_amount_for_price_impact could lead to inaccurate calculations if the market has a non-zero fee. Consider using the actual market fee:
max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact(
market, kelly_bet, fee=market.fee if hasattr(market, 'fee') else 0
)This change ensures that the fee is correctly accounted for in the price impact calculations.
| # We just don't want Kelly size to extrapolate price_impact - hence we take the min. | ||
| kelly_bet_size = min(kelly_bet.size, max_slippage_bet_amount) | ||
|
|
||
| self.assert_price_impact_lt_max_price_impact_else_raise( |
There was a problem hiding this comment.
Is this needed to run every time in the core code? Looks like a sanity check that could be done in a pytest, and removed from here.
There was a problem hiding this comment.
Makes sense. Refactored this as an integration test (see test_kelly).
|
|
||
| optimized_bet_amount = minimize_scalar( | ||
| calculate_price_impact_deviation_from_target_price_impact, | ||
| bounds=(min(yes_outcome_pool_size, no_outcome_pool_size) / 1000, 1000), |
There was a problem hiding this comment.
I think I commented on these bounds before, but find it. But these don't seem super robust, e.g. if used for markets with v large or v small liquidity (or non-Omen markets that use a different currency where it's normal to see larger/smaller values). Shouldn't the bounds be [0, inf]?
There was a problem hiding this comment.
Ah here's the previous comment #433 (comment)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
prediction_market_agent_tooling/tools/tavily_storage/tavily_models.py (1)
20-20: LGTM! Consider updating the import style for consistency.The change from using
loguruto an internal logging module is a good practice for maintaining control over the logging implementation. This change is unlikely to cause any functional issues as the usage oflogger.debug()is consistent with both implementations.For consistency with other imports in the file, consider using an absolute import:
from prediction_market_agent_tooling.loggers import loggerThis style is more explicit and less prone to potential circular import issues.
prediction_market_agent_tooling/deploy/betting_strategy.py (6)
146-149: LGTM: New parameter added for price impact controlThe addition of the
max_price_impactparameter with a default value ofNoneis a good improvement. It allows for more flexible betting strategies while maintaining backward compatibility.Consider adding a type hint for
max_price_impactto improve code readability:def __init__(self, max_bet_amount: float, max_price_impact: Optional[float] = None):Don't forget to import
Optionalfromtypingif you make this change.
150-174: LGTM: New method to enforce maximum price impactThis method effectively ensures that the price impact doesn't exceed the maximum allowed value. The use of
check_not_nonefor null checks andnp.isclosefor float comparisons are good practices.Consider adding a docstring to explain the purpose and parameters of this method. For example:
def assert_price_impact_lt_max_price_impact_else_raise( self, buy_direction: bool, bet_size: float, market: AgentMarket, ) -> None: """ Assert that the price impact of a bet is less than the maximum allowed price impact. Args: buy_direction (bool): True if buying, False if selling. bet_size (float): The size of the bet. market (AgentMarket): The market to check the price impact against. Raises: ValueError: If the price impact exceeds the maximum allowed value. """
204-217: LGTM: Bet size adjustment based on price impactThe addition of price impact considerations in the bet size calculation is a significant improvement. It ensures that the Kelly bet size doesn't exceed the maximum allowed price impact.
The hardcoded value of 0 for the fee parameter in
calculate_bet_amount_for_price_impactcould lead to inaccurate calculations if the market has a non-zero fee. Consider using the actual market fee:max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( market, kelly_bet, fee=market.fee if hasattr(market, 'fee') else 0 )
229-244: LGTM: New method for calculating price impactThe
calculate_price_impact_for_bet_amountmethod correctly implements the price impact calculation. The use ofget_buy_outcome_token_amountfrom the Omen market is appropriate.The comment "price_impact should always be > 0" might not always be true. Price impact can be negative if the actual price is lower than the expected price, which can happen in certain market conditions. Consider updating the comment to:
# Note: price_impact can be positive or negative depending on market conditions
246-287: LGTM: New method for optimizing bet amount based on price impactThe
calculate_bet_amount_for_price_impactmethod effectively uses numerical optimization to find the optimal bet amount. The use ofscipy.optimize.minimize_scalarwith bounded optimization is appropriate, and the error handling for missingoutcome_token_poolis good.Consider making the optimization bounds more flexible:
- The lower bound
min(yes_outcome_pool_size, no_outcome_pool_size) / 1000might be too large for markets with very small liquidity.- The upper bound of 1000 might be too small for markets with very large liquidity or when using a different currency.
You could make these bounds configurable or use a wider range. For example:
lower_bound = max(1e-6, min(yes_outcome_pool_size, no_outcome_pool_size) / 1e6) upper_bound = max(1000, max(yes_outcome_pool_size, no_outcome_pool_size) * 10) optimized_bet_amount = minimize_scalar( calculate_price_impact_deviation_from_target_price_impact, bounds=(lower_bound, upper_bound), method="bounded", tol=1e-11, options={"maxiter": 10000}, )This change would make the method more robust across different market conditions and currencies.
Line range hint
1-290: Overall assessment: Significant improvement in betting strategyThe changes to the
KellyBettingStrategyclass represent a substantial improvement in the betting strategy by incorporating price impact considerations. The new methods for calculating and optimizing bet amounts based on price impact are well-implemented and add valuable functionality to the strategy.Key improvements:
- Addition of
max_price_impactparameter for flexible control.- New methods for calculating and asserting price impact.
- Integration of price impact considerations in the
calculate_tradesmethod.- Use of numerical optimization to find optimal bet amounts.
These changes should lead to more sophisticated and potentially more profitable betting strategies, especially in markets where price impact is a significant factor.
To further improve the code:
- Consider extracting the price impact calculation logic into a separate class or module. This would improve modularity and make it easier to test and maintain the price impact logic independently of the betting strategy.
- Implement unit tests for the new methods, especially
calculate_price_impact_for_bet_amountandcalculate_bet_amount_for_price_impact, to ensure their correctness across various market conditions.- Consider adding logging statements in key parts of the code (e.g., when adjusting bet sizes or when price impact assertions fail) to aid in debugging and monitoring the strategy's performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- prediction_market_agent_tooling/deploy/betting_strategy.py (5 hunks)
- prediction_market_agent_tooling/markets/polymarket/api.py (1 hunks)
- prediction_market_agent_tooling/tools/is_predictable.py (1 hunks)
- prediction_market_agent_tooling/tools/tavily_storage/tavily_models.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- prediction_market_agent_tooling/markets/polymarket/api.py
🔇 Additional comments (3)
prediction_market_agent_tooling/tools/is_predictable.py (1)
Line range hint
1-185: LGTM! Ensure thorough testing of logging functionality.The change in the logger import doesn't appear to affect the rest of the file. The main functions
is_predictable_binaryandis_predictable_without_descriptionshould continue to work as expected. However, it's crucial to ensure that the logging functionality remains intact.To verify the logging functionality, please run the following test cases:
- Test the error logging in
is_predictable_binary:import sys sys.path.append('path/to/your/project') from prediction_market_agent_tooling.tools.is_predictable import is_predictable_binary # Simulate a scenario where langchain is not installed import sys sys.modules['langchain'] = None result = is_predictable_binary("Test question") # Check if the error was logged correctly
- Test the error logging in
is_predictable_without_description:from prediction_market_agent_tooling.tools.is_predictable import is_predictable_without_description # Simulate a scenario where langchain is not installed import sys sys.modules['langchain'] = None result = is_predictable_without_description("Test question", "Test description") # Check if the error was logged correctlyThese tests will help ensure that the logging functionality works as expected with the new logger.
prediction_market_agent_tooling/deploy/betting_strategy.py (2)
3-7: New imports added for price impact calculationsThe additions of
numpy,scipy.optimize, andget_buy_outcome_token_amountare appropriate for the new price impact calculations. These libraries provide the necessary tools for numerical optimization and financial calculations.
289-290: LGTM: Updated__repr__methodThe
__repr__method has been correctly updated to include themax_price_impactparameter. This change provides a more complete representation of the object's state, which is consistent with the addition ofmax_price_impactin the__init__method.
evangriffiths
changed the title
max_price_impact parameter to KellyBettingStrategy
|
|
||
| optimized_bet_amount = minimize_scalar( | ||
| calculate_price_impact_deviation_from_target_price_impact, | ||
| bounds=(min(yes_outcome_pool_size, no_outcome_pool_size) / 1000, 1000), |
There was a problem hiding this comment.
I see you get some exception with max bound of sys.maxsize. A simple way of making it more robust would be just to set the upper bound relative to total pool size.
I tried
| bounds=(min(yes_outcome_pool_size, no_outcome_pool_size) / 1000, 1000), | |
| bounds=(0, 1000 * (yes_outcome_pool_size + no_outcome_pool_size)), |
and it seems to work fine, and is more robust imo.
There was a problem hiding this comment.
Works for me as well. Implemented.
…track-experiments-kelly # Conflicts: # poetry.lock # pyproject.toml
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
prediction_market_agent_tooling/deploy/betting_strategy.py (6)
146-149: LGTM: New parameter for price impact control.The addition of
max_price_impactparameter allows for configurable price impact limits, which is a good enhancement to the strategy.Consider using
Optional[float]instead offloat | Nonefor better compatibility with Python versions below 3.10:-def __init__(self, max_bet_amount: float, max_price_impact: float | None = None): +from typing import Optional + +def __init__(self, max_bet_amount: float, max_price_impact: Optional[float] = None):
150-174: LGTM: Price impact assertion method.This method effectively ensures that the price impact doesn't exceed the specified limit. The use of
np.iscloseis a good practice for floating-point comparisons.Consider the following improvements:
- Make the tolerance configurable, e.g., as a class attribute.
- Enhance the error message to include the actual difference between the price impact and the max_price_impact.
class KellyBettingStrategy(BettingStrategy): def __init__(self, max_bet_amount: float, max_price_impact: Optional[float] = None, price_impact_tolerance: float = 0.01): # ... existing code ... self.price_impact_tolerance = price_impact_tolerance def assert_price_impact_lt_max_price_impact_else_raise( self, buy_direction: bool, bet_size: float, market: AgentMarket, ) -> None: # ... existing code ... if price_impact > max_price_impact and not np.isclose( price_impact, max_price_impact, atol=max_price_impact * self.price_impact_tolerance ): raise ValueError( f"Price impact {price_impact:.6f} exceeds max_price_impact {max_price_impact:.6f} " f"by {price_impact - max_price_impact:.6f} (tolerance: {max_price_impact * self.price_impact_tolerance:.6f}), " f"market_id {market.id}" )
204-217: LGTM: Bet size adjustment based on price impact.The addition of price impact consideration in the bet size calculation is a significant improvement. It ensures that the Kelly bet size doesn't exceed the maximum allowed price impact.
Consider the following improvements:
- Remove the redundant assertion after adjustment, as
calculate_bet_amount_for_price_impactshould already ensure the price impact is within limits.- Rename
max_slippage_bet_amounttomax_price_impact_bet_amountfor consistency with the new terminology.- Pass the market fee to
calculate_bet_amount_for_price_impactinstead of hardcoding 0.if self.max_price_impact: # Adjust amount - max_slippage_bet_amount = self.calculate_bet_amount_for_price_impact( - market, kelly_bet, 0 + max_price_impact_bet_amount = self.calculate_bet_amount_for_price_impact( + market, kelly_bet, market.fee ) # We just don't want Kelly size to extrapolate price_impact - hence we take the min. - kelly_bet_size = min(kelly_bet.size, max_slippage_bet_amount) + kelly_bet_size = min(kelly_bet.size, max_price_impact_bet_amount) - self.assert_price_impact_lt_max_price_impact_else_raise( - kelly_bet.direction, kelly_bet_size, market - )
229-244: LGTM: Price impact calculation method.The
calculate_price_impact_for_bet_amountmethod correctly calculates the price impact based on the expected and actual prices.Update the comment about price impact always being > 0, as it might not be accurate in all cases:
-# price_impact should always be > 0 +# Note: price_impact can be positive or negative depending on market conditions price_impact = (actual_price - expected_price) / expected_price
246-288: LGTM: Optimal bet amount calculation for price impact.The
calculate_bet_amount_for_price_impactmethod effectively uses numerical optimization to find the bet amount that achieves the target price impact.Consider the following improvements:
- Make the optimization bounds more flexible:
-bounds=(0, 1000 * (yes_outcome_pool_size + no_outcome_pool_size)), +bounds=(0, 10 * (yes_outcome_pool_size + no_outcome_pool_size)),
- Add a docstring explaining the method's purpose and parameters.
- Consider extracting the inner function
calculate_price_impact_deviation_from_target_price_impactas a separate method for better readability.- Add error handling for cases where the optimization fails to converge.
Example implementation:
def calculate_bet_amount_for_price_impact( self, market: AgentMarket, kelly_bet: SimpleBet, fee: float, ) -> float: """ Calculate the optimal bet amount that achieves the target price impact. Args: market (AgentMarket): The market to calculate the bet amount for. kelly_bet (SimpleBet): The Kelly bet object containing direction and size. fee (float): The market fee. Returns: float: The optimal bet amount that achieves the target price impact. Raises: ValueError: If the optimization fails to converge. """ if not market.outcome_token_pool: logger.warning("Market outcome_token_pool is None, cannot calculate bet amount") return kelly_bet.size yes_outcome_pool_size = market.outcome_token_pool[market.get_outcome_str_from_bool(True)] no_outcome_pool_size = market.outcome_token_pool[market.get_outcome_str_from_bool(False)] result = minimize_scalar( self._calculate_price_impact_deviation, args=(kelly_bet, yes_outcome_pool_size, no_outcome_pool_size, fee), bounds=(0, 10 * (yes_outcome_pool_size + no_outcome_pool_size)), method="bounded", tol=1e-11, options={"maxiter": 10000}, ) if not result.success: raise ValueError(f"Failed to find optimal bet amount: {result.message}") return float(result.x) def _calculate_price_impact_deviation( self, bet_amount: float, kelly_bet: SimpleBet, yes_outcome_pool_size: float, no_outcome_pool_size: float, fee: float, ) -> float: price_impact = self.calculate_price_impact_for_bet_amount( kelly_bet.direction, bet_amount, yes_outcome_pool_size, no_outcome_pool_size, fee, ) max_price_impact = check_not_none(self.max_price_impact) return abs(price_impact - max_price_impact)
Line range hint
1-291: Overall assessment: Significant enhancement to KellyBettingStrategyThe changes introduce price impact considerations to the KellyBettingStrategy, which is a valuable improvement. The implementation is generally well-structured and efficient, using numerical optimization techniques to calculate optimal bet amounts.
Key improvements:
- Addition of
max_price_impactparameter for configurable price impact limits.- New methods for calculating and asserting price impacts.
- Adjustment of bet sizes based on price impact constraints.
While the implementation is sound, consider the following general suggestions:
- Improve error handling and logging throughout the new methods.
- Add comprehensive unit tests for the new functionality, especially edge cases.
- Consider adding more detailed documentation, including examples of how to use the new price impact features.
To further enhance the strategy's robustness and flexibility, consider:
- Implementing a factory method or configuration system to easily create different variants of the KellyBettingStrategy with various price impact settings.
- Exploring the possibility of dynamically adjusting the
max_price_impactbased on market conditions or historical data.- Implementing a simulation framework to test the strategy's performance under various market scenarios and price impact settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
poetry.lockis excluded by!**/*.lock,!**/*.lockpyproject.tomlis excluded by!**/*.toml
📒 Files selected for processing (1)
- prediction_market_agent_tooling/deploy/betting_strategy.py (5 hunks)
🔇 Additional comments (2)
prediction_market_agent_tooling/deploy/betting_strategy.py (2)
3-4: LGTM: New imports for numerical operations.The addition of numpy and scipy.optimize.minimize_scalar is appropriate for the new price impact calculations implemented in the
KellyBettingStrategyclass.
291-291: LGTM: Updated string representation.The
__repr__method now includes themax_price_impactparameter, providing a more complete representation of the strategy's configuration.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
tests_integration/markets/omen/test_kelly.py (5)
16-23: LGTM: Well-documented test case for price impact calculation.The test case is clear and references the Gnosis documentation, which is excellent for traceability. The use of assert_price_impact is appropriate for verifying the calculation.
Consider adding a brief comment explaining the expected outcome of this test case to enhance readability.
26-34: LGTM: Consistent follow-up test case for price impact calculation.This test case logically follows the previous one and continues to reference the Gnosis documentation. The use of assert_price_impact is consistent with the previous test.
For consistency with the previous test, consider adding a brief comment explaining the expected outcome of this specific scenario.
37-50: LGTM: Well-structured parameterized test for large pool price impact.The use of pytest.mark.parametrize is excellent for testing multiple scenarios efficiently. The test appropriately retrieves a large market for realistic testing.
Consider the following improvements:
- Add error handling for potential exceptions from OmenSubgraphHandler.get_omen_binary_markets_simple().
- Include a brief comment explaining the purpose of testing with a large pool and the expected outcomes for different parameter combinations.
69-108: LGTM: Well-structured helper function for price impact convergence.The function effectively calculates and asserts the convergence of price impact. The use of np.isclose for floating-point comparison is appropriate.
Consider the following improvements:
- Add error handling for potential exceptions from external function calls (e.g., get_kelly_bet_full, calculate_bet_amount_for_price_impact).
- Consider adding a docstring to explain the function's purpose and parameters.
- The tolerance in the assertion (0.001 * max_price_impact) might be too strict for some cases. Consider making it a parameter or using a relative tolerance instead.
111-135: LGTM: Well-implemented helper function for price impact assertion.The function effectively calculates and asserts the correctness of price impact for both buy and sell directions. The logic is clear and the use of np.isclose for comparison is appropriate.
Consider the following improvements:
- Remove the print statement at the end of the function (line 135) as it's not necessary for a test function and might clutter the test output.
- Add a docstring to explain the function's purpose and parameters.
- Consider using a constant for the relative tolerance in np.isclose (currently 0.01) to make it easier to adjust if needed.
prediction_market_agent_tooling/deploy/betting_strategy.py (2)
145-147: LGTM: Newmax_price_impactparameter addedThe addition of the
max_price_impactparameter with a default value ofNoneis a good improvement. It allows for more flexible betting strategies while maintaining backward compatibility.Consider adding a docstring to explain the purpose and usage of the
max_price_impactparameter.
215-257: New method for calculating bet amount based on price impact is well-implementedThe
calculate_bet_amount_for_price_impactmethod is a good addition, using numerical optimization to find the bet amount that achieves the target price impact.Consider the following improvements:
- Add a docstring explaining the purpose and parameters of the method.
- Add a comment explaining the rationale behind the chosen optimization bounds.
- Consider making the bounds more flexible or configurable, especially for markets with very large or very small liquidity pools.
Example docstring:
def calculate_bet_amount_for_price_impact( self, market: AgentMarket, kelly_bet: SimpleBet, fee: float, ) -> float: """ Calculate the optimal bet amount that achieves the target price impact. Args: market (AgentMarket): The market to calculate the bet amount for. kelly_bet (SimpleBet): The Kelly bet object containing direction and size. fee (float): The market fee. Returns: float: The optimal bet amount that achieves the target price impact. """prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py (1)
322-331: Summary: New liquidity-based sorting options added.The changes introduce new sorting options for
HIGHEST_LIQUIDITYandLOWEST_LIQUIDITYin the_build_sort_paramsmethod. These additions enhance the functionality of the Omen subgraph handler by allowing markets to be sorted based on their liquidity.To ensure these changes are fully integrated:
- Verify that the
get_omen_binary_markets_simplemethod properly handles these new sorting options.- Update any relevant documentation or API references to include these new sorting options.
- Consider adding unit tests to verify the behavior of these new sorting options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- prediction_market_agent_tooling/deploy/betting_strategy.py (5 hunks)
- prediction_market_agent_tooling/markets/agent_market.py (1 hunks)
- prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py (1 hunks)
- tests_integration/markets/omen/test_kelly.py (1 hunks)
🔇 Additional comments (8)
tests_integration/markets/omen/test_kelly.py (2)
1-14: LGTM: Import statements are well-organized and appropriate.The import statements are logically organized and include all necessary modules for testing the KellyBettingStrategy. Good job on following the import order convention (standard library, third-party, local).
1-135: Overall: Well-structured test file with room for minor improvementsThis test file for the KellyBettingStrategy is generally well-organized and covers various scenarios for price impact calculations. The use of parameterized tests and helper functions promotes code reuse and maintainability.
Key points for improvement:
- Address the inconsistency in the test_kelly_price_impact_works_small_pool function.
- Add error handling for potential exceptions from external API calls and function invocations.
- Enhance documentation with docstrings for helper functions and brief comments explaining expected outcomes for test cases.
- Remove unnecessary print statements and consider parameterizing tolerance values for easier adjustment.
Addressing these points will further improve the robustness and clarity of the tests.
prediction_market_agent_tooling/markets/agent_market.py (2)
Line range hint
1-32: Overall assessment: Changes look good, but seem to be part of a larger implementation.The additions to the
SortByenum are well-implemented and consistent with the existing code. However, these changes alone don't fully implement themax_price_impactparameter mentioned in the PR objectives. It would be helpful to see the complete set of changes across all affected files to fully understand how this enhancement to the Kelly betting strategy is being implemented.To get a better understanding of the full scope of changes, please run the following script:
#!/bin/bash # Description: List all modified files in this PR and show a summary of changes # List all modified files echo "Modified files in this PR:" git diff --name-only origin/main # Show a summary of changes for each file echo "\nSummary of changes:" git diff --stat origin/mainThis will help reviewers understand the full context of the changes and how they relate to the PR objectives.
31-32: LGTM! Consider adding documentation for the new sorting options.The addition of
HIGHEST_LIQUIDITYandLOWEST_LIQUIDITYto theSortByenum is a good enhancement. These new sorting options align well with the PR's objective of improving the Kelly betting strategy by considering market conditions.Consider adding a brief comment above the
SortByclass to explain the purpose of these sorting options, especially the new liquidity-based ones. This would help other developers understand the intent and usage of these options.To ensure these changes are properly integrated, please run the following script to check for any usage of
SortBythat might need updating:✅ Verification successful
Verification Successful
All usages of the
SortByenum, including the newHIGHEST_LIQUIDITYandLOWEST_LIQUIDITYvalues, have been reviewed and integrated correctly across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of SortBy enum and potential areas that need updating # Search for SortBy usage echo "Searching for SortBy usage:" rg --type python "SortBy\." -A 5 # Search for potential switch statements or if-else blocks that might need updating echo "\nSearching for potential switch statements or if-else blocks using SortBy:" rg --type python "if.*SortBy\..*:.*elif.*SortBy\." -A 10Length of output: 1588
Script:
#!/bin/bash # Description: Check for usage of SortBy enum and potential areas that need updating # Search for SortBy usage echo "Searching for SortBy usage:" rg --type python "SortBy\." -A 5 # Search for potential switch statements or if-else blocks using SortBy echo -e "\nSearching for potential switch statements or if-else blocks using SortBy:" rg --type python "if\s+.*SortBy\..*:" -A 10Length of output: 370
Script:
#!/bin/bash # Description: Check for usage of SortBy enum and potential areas that need updating # Search for SortBy usage echo "Searching for SortBy usage:" rg --type py "SortBy\." -A 5 # Search for potential if-else blocks using SortBy echo -e "\nSearching for potential if-else blocks using SortBy:" rg --type py "if\s+.*SortBy\..*:" -A 10Length of output: 23556
prediction_market_agent_tooling/deploy/betting_strategy.py (3)
3-6: New imports added for price impact calculationsThe new imports (
minimize_scalar,xDai, andlogger) are appropriate for the added functionality. Theminimize_scalarfunction from SciPy will be used for optimizing the bet amount based on price impact.
260-260: LGTM: Updated__repr__methodThe
__repr__method has been correctly updated to include themax_price_impactparameter in the string representation of theKellyBettingStrategyobject.
198-213:⚠️ Potential issueNew method for calculating price impact looks good
The
calculate_price_impact_for_bet_amountmethod is well-implemented and correctly calculates the price impact for a given bet amount.However, the comment "price_impact should always be > 0" on line 211 is not always true. Price impact can be negative if the actual price is lower than the expected price. Consider updating the comment to reflect this possibility:
-# price_impact should always be > 0 +# Note: price_impact can be positive or negative depending on market conditionsLikely invalid or redundant comment.
prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py (1)
322-331: New sorting options added for market liquidity.The changes introduce two new sorting options:
HIGHEST_LIQUIDITYandLOWEST_LIQUIDITY. These additions enhance the flexibility of the market querying functionality, allowing users to sort markets based on their liquidity.A few observations:
- The implementation is consistent with the existing pattern.
- The new options use the
liquidityMeasurefield, which is appropriate for sorting by liquidity.- The changes don't break existing functionality.
No description provided.