Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compare agent bet histories with kelly strategy #419
Compare agent bet histories with kelly strategy #419
Changes from all commits
e66572a
e48aea3
48e8993
2325c4b
b78489f
0ab2ef5
aeffaa9
2cbb236
1212dca
c6ee7fe
82c103b
b34020b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Consider improving error handling for mismatched bets and traces.
The data retrieval and processing logic is well-implemented. Matching bets with corresponding traces and checking for data integrity are crucial steps.
The current error handling for mismatched bets and traces could be improved. Instead of raising a
ValueError
, consider logging a warning and continuing with the available data. This approach would allow the script to process as much data as possible, even if some bets are missing traces.Replace the following code:
with:
This change will allow the script to continue processing even if some bets are missing traces, providing more robust behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Consider adding summary statistics for easier comparison.
The simulation logic and results presentation are well-implemented. The comparison of ROI between actual bets and various simulated strategies provides valuable insights.
To enhance the analysis of results, consider adding summary statistics to compare the performance of different strategies more easily. You could add the following code after the DataFrame is created:
This addition will provide a concise overview of each strategy's performance, making it easier to compare and identify the most effective approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure
max_bet_amount
enforces the intended maximumIn the
adjust_bet_amount
method, addingexisting_position_total_amount
toself.max_bet_amount
might result in a total bet exceedingmax_bet_amount
when there's an existing position. Ifmax_bet_amount
is intended to cap the total bet amount, consider revising the calculation to enforce this limit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using actual estimated probabilities and confidence
Currently,
estimated_p_yes
is set to1.0
or0.0
based on whetheranswer.p_yes > 0.5
, andconfidence
is hardcoded to1.0
. This may not fully utilize the model's probability estimates and confidence levels. To leverage the Kelly criterion effectively, consider using the actualanswer.p_yes
andanswer.confidence
values.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor shared logic into a common method
The
calculate_trades
method contains logic similar to that inKellyBettingStrategy
. To promote code reusability and reduce duplication, consider refactoring the common code for calculating the Kelly bet into a shared method or utility function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider Removing Hardcoded Default
project_id
Hardcoding the
project_id
as a default parameter reduces the function's flexibility and may lead to issues in different environments or projects. It's better to requireproject_id
as a mandatory parameter or retrieve it from an environment variable or configuration file.Apply this diff to make
project_id
a required parameter:Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Exception Handling for Secret Manager Operations
Accessing secrets from GCP Secret Manager can raise exceptions due to network errors, authentication issues, or missing permissions. To improve robustness, add exception handling to gracefully manage potential errors.
Wrap the secret access code in a try-except block: