-
Notifications
You must be signed in to change notification settings - Fork 2
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
Monitor market resolutions #16
Conversation
Warning Rate Limit Exceeded@kongzii has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 48 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates focus on enhancing the monitoring and benchmarking capabilities of prediction market tooling. Key changes include the refactoring of Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
poetry.lock
is excluded by:!**/*.lock
pyproject.toml
is excluded by:!**/*.toml
Files selected for processing (6)
- .gitignore (1 hunks)
- examples/monitor/monitor.py (1 hunks)
- prediction_market_agent_tooling/benchmark/utils.py (7 hunks)
- prediction_market_agent_tooling/markets/manifold.py (1 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (4 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (1 hunks)
Additional comments: 11
examples/monitor/monitor.py (1)
- 3-8: The refactoring to use
monitor_app
frommonitor_app.py
is correctly implemented. The import statement and the usage within the main block follow Python's standard practices, enhancing modularity and maintainability of the code.prediction_market_agent_tooling/monitor/monitor_app.py (2)
- 20-23: The
MonitorSettings
class correctly defines settings for the monitoring application, including the use of an environment file. This approach enhances configurability and maintainability.- 65-68: The loop for loading Manifold agents and appending them to a list is correctly implemented. However, ensure that the
get_authenticated_user
function gracefully handles any potential errors or exceptions that might occur during the API call.Ensure robust error handling in the
get_authenticated_user
function to prevent the application from crashing due to API call failures..gitignore (1)
- 124-124: The addition of
.env.monitor
to the.gitignore
file is a good practice, ensuring that environment-specific settings are kept out of version control. This enhances the security and maintainability of the project.prediction_market_agent_tooling/markets/manifold.py (1)
- 84-87: The modification to the
get_authenticated_user
function to require anapi_key
parameter is a positive change, enhancing flexibility and security by allowing different API keys to be used. This follows best practices for API usage.prediction_market_agent_tooling/monitor/monitor.py (2)
- 3-6: The addition of imports for
numpy
,groupby
, and other necessary modules supports the new functionality introduced in this file, such as data analysis and chart generation.- 28-30: The enhancement to the
monitor_agent
function to handle cases where no resolved bets are found by displaying a warning message is a thoughtful addition, improving user experience by providing informative feedback.prediction_market_agent_tooling/benchmark/utils.py (4)
- 3-6: The addition of
datetime
andpytz
imports supports the new functionality for handling date and time operations, aligning with the enhancements made to market data fetching.- 10-11: The introduction of the
MANIFOLD_API_LIMIT
constant is a good practice, providing a clear and modifiable limit for API requests. This enhances maintainability and readability of the code.- 39-50: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [30-46]
The enhancements to the
Market
class, including the addition of thecreated_time
field and its validation, improve the model's accuracy and reliability by ensuring that date and time values are correctly handled.
- 159-209: The introduction of
get_manifold_markets_paged
andget_manifold_markets_dated
functions significantly enhances the tool's ability to fetch market data more efficiently and comprehensively. These functions introduce pagination and date-based fetching, allowing for more precise and efficient data retrieval. Ensure that these functions are thoroughly tested, especially the logic for breaking out of loops and handling date comparisons.Thoroughly test the
get_manifold_markets_paged
andget_manifold_markets_dated
functions to ensure the pagination and date-based fetching logic correctly handles all edge cases.
def monitor_app() -> None: | ||
settings = MonitorSettings() | ||
start_time = datetime.combine( | ||
t.cast( | ||
# This will be always a date for us, so casting. | ||
date, | ||
st.date_input( | ||
"Start time", | ||
value=datetime.now() - timedelta(weeks=settings.PAST_N_WEEKS), | ||
), | ||
), | ||
datetime.min.time(), | ||
).replace(tzinfo=pytz.UTC) | ||
market_type: MarketType = check_not_none( | ||
st.selectbox(label="Market type", options=list(MarketType), index=0) | ||
) | ||
|
||
if market_type != MarketType.MANIFOLD: | ||
st.warning("Only Manifold markets are supported for now.") | ||
return | ||
|
||
st.subheader("Market resolution") | ||
monitor_market(start_time, market_type) | ||
|
||
with st.spinner("Loading Manifold agents..."): | ||
agents: list[DeployedManifoldAgent] = [] | ||
for key in settings.MANIFOLD_API_KEYS: | ||
manifold_user = get_authenticated_user(key) | ||
agents.append( | ||
DeployedManifoldAgent( | ||
name=manifold_user.name, | ||
manifold_user_id=manifold_user.id, | ||
start_time=start_time, | ||
) | ||
) | ||
|
||
st.subheader("Agent bets") | ||
for agent in agents: | ||
with st.expander(f"Agent: '{agent.name}'"): | ||
monitor_agent(agent) |
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.
The monitor_app
function is well-structured and modular, correctly initializing settings, handling date input, and supporting market type selection. However, it currently only supports Manifold markets, as indicated by the warning message in lines 46-48. Consider adding support for other market types in the future to enhance the tool's versatility.
Consider planning for the extension to support other market types beyond Manifold to increase the tool's applicability.
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.
Niiiice! Same again though about market-specific code
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.
@evangriffiths, I appreciate your focus on making the tool more versatile by addressing market-specific code concerns. A good strategy to accommodate various market types while maintaining the current functionality for Manifold markets could involve:
-
Interface Abstraction: Define an interface (or abstract class) that outlines common operations for market interactions, such as fetching market data, monitoring markets, and handling agent bets. Each market type (Manifold, and others in the future) would implement this interface according to its specific API and logic.
-
Factory Pattern: Implement a factory method or class that instantiates the appropriate market-specific class based on configuration or runtime conditions. This allows the
monitor_app
function to interact with market objects through the common interface, decoupling it from specific market implementations. -
Modularization: Organize market-specific logic into separate modules or classes within a dedicated directory (e.g.,
markets
). This structure makes it easier to add or modify implementations for different market types without impacting the core monitoring functionality.
By adopting these strategies, you can enhance the tool's applicability and ease the integration of additional market types in the future.
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.
I will add them when we have the necessary support in Polymarket's API.
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
def monitor_market(start_time: datetime, market_type: MarketType) -> None: | ||
assert ( | ||
market_type == MarketType.MANIFOLD | ||
), "TODO: Add support for other market types." | ||
with st.spinner("Loading market data..."): | ||
open_markets = get_manifold_markets_dated( | ||
oldest_date=start_time, filter_="open" | ||
) | ||
resolved_markets = [ | ||
m | ||
for m in get_manifold_markets_dated( | ||
oldest_date=start_time, filter_="resolved" | ||
) | ||
if m.resolution not in ("CANCEL", "MKT") | ||
] | ||
|
||
date_to_open_yes_proportion = { | ||
d: np.mean([int(m.p_yes > 0.5) for m in markets]) | ||
for d, markets in groupby(open_markets, lambda x: x.created_time.date()) | ||
} | ||
date_to_resolved_yes_proportion = { | ||
d: np.mean( | ||
[ | ||
( | ||
1 | ||
if m.resolution == "YES" | ||
else ( | ||
0 | ||
if m.resolution == "NO" | ||
else should_not_happen(f"Unexpected resolution: {m.resolution}") | ||
) | ||
) | ||
for m in markets | ||
] | ||
) | ||
for d, markets in groupby(resolved_markets, lambda x: x.created_time.date()) | ||
} | ||
|
||
df_open = pd.DataFrame( | ||
date_to_open_yes_proportion.items(), columns=["date", "open_proportion"] | ||
) | ||
df_open["open_label"] = "Open's yes proportion" | ||
df_resolved = pd.DataFrame( | ||
date_to_resolved_yes_proportion.items(), columns=["date", "resolved_proportion"] | ||
) | ||
df_resolved["resolved_label"] = "Resolved's yes proportion" | ||
|
||
df = pd.merge(df_open, df_resolved, on="date") | ||
|
||
open_chart = ( | ||
alt.Chart(df) | ||
.mark_line() | ||
.encode(x="date:T", y="open_proportion:Q", color="open_label:N") | ||
) | ||
|
||
resolved_chart = ( | ||
alt.Chart(df) | ||
.mark_line() | ||
.encode(x="date:T", y="resolved_proportion:Q", color="resolved_label:N") | ||
) | ||
|
||
st.altair_chart( | ||
alt.layer(open_chart, resolved_chart).interactive(), # type: ignore # Doesn't expect `LayerChart`, but `Chart`, yet it works. | ||
use_container_width=True, | ||
) | ||
|
||
all_open_markets_yes_mean = np.mean([int(m.p_yes > 0.5) for m in open_markets]) | ||
all_resolved_markets_yes_mean = np.mean( | ||
[ | ||
( | ||
1 | ||
if m.resolution == "YES" | ||
else ( | ||
0 | ||
if m.resolution == "NO" | ||
else should_not_happen(f"Unexpected resolution: {m.resolution}") | ||
) | ||
) | ||
for m in resolved_markets | ||
] | ||
) | ||
st.markdown( | ||
f"Total number of open markets {len(open_markets)} and resolved markets {len(resolved_markets)}" | ||
"\n\n" | ||
f"Mean proportion of 'YES' in open markets: {all_open_markets_yes_mean:.2f}" | ||
"\n\n" | ||
f"Mean proportion of 'YES' in resolved markets: {all_resolved_markets_yes_mean:.2f}" | ||
) |
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.
The monitor_market
function is well-implemented, providing detailed monitoring based on start time and market type. It includes logic for loading market data, calculating proportions, and displaying charts. However, it currently asserts support only for Manifold markets. Consider planning for the extension to support other market types to increase the tool's applicability.
Consider extending support to other market types beyond Manifold in the monitor_market
function to enhance the tool's versatility.
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.
Agree with coderabbit :). Can we keep monitor/monitor.py free of any market-specific code. Should be straight forward in this case as get_manifold_markets_dated
returns t.List[Market]
.
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.
@evangriffiths, I'm glad you agree with the initial suggestion and appreciate your guidance on keeping monitor/monitor.py
free of market-specific code. Your point about get_manifold_markets_dated
returning t.List[Market]
is well-taken and indeed paves the way for a more generic implementation.
To align with your suggestion, a potential approach could involve:
- Reviewing the
monitor_market
function to identify and abstract any Manifold-specific logic, ensuring it operates on genericMarket
objects. - Considering the introduction of a generic interface or utility functions that can handle fetching and processing market data in a market-agnostic manner. This could involve abstracting the logic that currently assumes Manifold-specific fields or behaviors.
- Ensuring that any new abstraction accommodates the needs of different market types without compromising on functionality or detail.
This refactor would not only enhance the tool's versatility but also its maintainability and scalability, making it easier to integrate new market types in the future.
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.
Sure, refactored to def monitor_market(open_markets: list[Market], resolved_markets: list[Market]) -> None:
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
@validator("created_time") | ||
def _validate_created_time(cls, value: datetime) -> datetime: | ||
if value.tzinfo is None: | ||
value = value.replace(tzinfo=pytz.UTC) |
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.
We need timezone-aware datetime to compare it with others.
|
||
|
||
def get_manifold_markets( | ||
number: int = 100, |
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.
I added offset
, so I renamed this to limit
to adhere to the API convention.
|
||
|
||
def get_manifold_markets( | ||
number: int = 100, | ||
excluded_questions: t.List[str] = [], |
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.
This wasn't used anywhere and it would make logic for the two new functions harder, but lmk if we need it
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.
😢 I was using it here gnosis/mech#1. Am I missing something - can't you just add it as an arg to the two new functions and pass it in from here?
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.
Oops sorry! Sure added!
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.
Thanks
active: bool | None = True, | ||
closed: bool | None = False, | ||
) -> t.List[Market]: | ||
params: dict[str, str | int] = { | ||
"_limit": number + len(excluded_questions), | ||
"_limit": 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.
Changed to be on par with manifold logic. I wanted to implement the same new functions for polymarket, but:
- https://clob.polymarket.com//markets returns next_cursor, but can'f filter on market status
- https://strapi-matic.poly.market/markets allows to filter, but doesn't return next_cursor
Who did you ask for the special endpoint? I can ask if they can make it paged as well.
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.
Ah that's annoying. It was pre-existing and I just found it by googling.
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.
Then let's leave it for now, and I will take a look in the next PR.
@@ -47,7 +55,6 @@ def monitor_agent(agent: DeployedAgent) -> None: | |||
profit_df.groupby("Date")["Cumulative Profit"].sum().cumsum().reset_index() | |||
) | |||
profit_df["Cumulative Profit"] = profit_df["Cumulative Profit"].astype(float) | |||
st.empty() |
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.
Noticed these two empty functions. Empty is used as a placeholder, for example we could do
placeholder = st.empty()
# Some logic
...
# Write something as usual
st.write(...)
# Use the placeholder, now this would be actually written first at the page.
placeholder.write(...)
But alone like this it should do nothing. Sorry I didn't catch it during the review!
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.
Ah my bad. I meant it for vertical whitespace. Looks like that should be st.write('\n')
instead
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- prediction_market_agent_tooling/benchmark/utils.py (8 hunks)
- prediction_market_agent_tooling/monitor/monitor.py (4 hunks)
- prediction_market_agent_tooling/monitor/monitor_app.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- prediction_market_agent_tooling/monitor/monitor_app.py
Additional comments: 11
prediction_market_agent_tooling/monitor/monitor.py (3)
- 3-3: The import of
groupby
fromitertools
is a good addition for efficient data grouping operations. Ensure that its usage in the newmonitor_market
function follows best practices for handling iterables.- 6-6: The addition of
numpy
is appropriate for numerical operations, especially for calculating proportions in themonitor_market
function. Verify thatnumpy
operations are used efficiently and correctly throughout the function.- 27-29: Enhancing the
monitor_agent
function with a warning message when no resolved bets are found improves user feedback and experience. This change is a good practice for handling empty data scenarios.prediction_market_agent_tooling/benchmark/utils.py (8)
- 3-3: The addition of the
datetime
import is necessary for handling date and time operations, especially for the newcreated_time
field in theMarket
class.- 6-6: The import of
pytz
is appropriate for timezone handling, ensuring thatcreated_time
in theMarket
class is timezone-aware. This is crucial for consistent date-time operations across different time zones.- 39-50: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [30-46]
The addition of the
created_time
field and its validator in theMarket
class enhances the model by ensuring that all market instances have a timezone-awaredatetime
object. This is a good practice for data consistency and avoids potential issues with timezone-naive datetime objects.
- 121-126: The modification of the
get_manifold_markets
function to includelimit
,offset
, andsort
parameters improves the flexibility of market data fetching. This allows for more efficient data retrieval by fetching only the necessary amount of data and sorting it as needed.- 159-187: The addition of the
get_manifold_markets_paged
function is a significant improvement for fetching large datasets in pages. This approach reduces the load on the API and ensures that the application can handle large amounts of data efficiently. Ensure that the logic for handling theexcluded_questions
parameter and pagination is correctly implemented.- 190-215: The
get_manifold_markets_dated
function introduces functionality to fetch markets based on date, which is a valuable addition for time-based data analysis. Ensure that the date comparison logic (market.created_time < oldest_date
) correctly handles timezone-aware datetime objects.- 145-229: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [220-251]
The modifications to the
get_polymarket_markets
function, including the addition of alimit
parameter and the removal of handling for excluded questions, streamline the function. However, the removal of excluded questions handling might impact functionality if it was previously used to filter out specific markets. Verify that this change aligns with the intended use cases and does not inadvertently remove necessary functionality.Verification successful
The search results indicate that the
get_polymarket_markets
function is only used within its own definition in theprediction_market_agent_tooling/benchmark/utils.py
file, with no external calls found. This suggests that the modifications, including the addition of alimit
parameter and the removal of handling for excluded questions, are self-contained. Therefore, the concern about inadvertently removing necessary functionality due to these changes does not apply to external dependencies or use cases within the codebase. The impact of these modifications appears to be limited to the function itself, aligning with the intended use cases as long as the internal logic of the function is correctly implemented.* 264-270: The modification of the `get_markets` function to remove the `excluded_questions` parameter simplifies the function signature. Ensure that all calls to this function have been updated accordingly to avoid passing the now-removed parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of get_polymarket_markets to verify the impact of removing excluded questions handling. rg "get_polymarket_markets" --type pythonLength of output: 72
Script:
#!/bin/bash # Corrected search for usages of get_polymarket_markets across Python files without using the --type option. rg "get_polymarket_markets" "*.py"Length of output: 79
Script:
#!/bin/bash # Correctly search for usages of get_polymarket_markets across Python files using the -g option for glob patterns. rg "get_polymarket_markets" -g "*.py"Length of output: 206
Verification successful
The verification process has confirmed that calls to
get_markets
within the codebase have been updated accordingly, and theexcluded_questions
parameter is not being passed. This aligns with the modifications made to theget_markets
function, ensuring compatibility with its updated signature. No issues were found related to the removal of theexcluded_questions
parameter.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to get_markets to ensure the removed parameter is not being passed. rg "get_markets" --type pythonLength of output: 61
Script:
#!/bin/bash # Correcting the file type and searching for calls to get_markets within Python files. rg "get_markets" --type pyLength of output: 715
def monitor_market(open_markets: list[Market], resolved_markets: list[Market]) -> None: | ||
date_to_open_yes_proportion = { | ||
d: np.mean([int(m.p_yes > 0.5) for m in markets]) | ||
for d, markets in groupby(open_markets, lambda x: x.created_time.date()) | ||
} | ||
date_to_resolved_yes_proportion = { | ||
d: np.mean( | ||
[ | ||
( | ||
1 | ||
if m.resolution == "YES" | ||
else ( | ||
0 | ||
if m.resolution == "NO" | ||
else should_not_happen(f"Unexpected resolution: {m.resolution}") | ||
) | ||
) | ||
for m in markets | ||
] | ||
) | ||
for d, markets in groupby(resolved_markets, lambda x: x.created_time.date()) | ||
} | ||
|
||
df_open = pd.DataFrame( | ||
date_to_open_yes_proportion.items(), columns=["date", "open_proportion"] | ||
) | ||
df_open["open_label"] = "Open's yes proportion" | ||
df_resolved = pd.DataFrame( | ||
date_to_resolved_yes_proportion.items(), columns=["date", "resolved_proportion"] | ||
) | ||
df_resolved["resolved_label"] = "Resolved's yes proportion" | ||
|
||
df = pd.merge(df_open, df_resolved, on="date") | ||
|
||
open_chart = ( | ||
alt.Chart(df) | ||
.mark_line() | ||
.encode(x="date:T", y="open_proportion:Q", color="open_label:N") | ||
) | ||
|
||
resolved_chart = ( | ||
alt.Chart(df) | ||
.mark_line() | ||
.encode(x="date:T", y="resolved_proportion:Q", color="resolved_label:N") | ||
) | ||
|
||
st.altair_chart( | ||
alt.layer(open_chart, resolved_chart).interactive(), # type: ignore # Doesn't expect `LayerChart`, but `Chart`, yet it works. | ||
use_container_width=True, | ||
) | ||
|
||
all_open_markets_yes_mean = np.mean([int(m.p_yes > 0.5) for m in open_markets]) | ||
all_resolved_markets_yes_mean = np.mean( | ||
[ | ||
( | ||
1 | ||
if m.resolution == "YES" | ||
else ( | ||
0 | ||
if m.resolution == "NO" | ||
else should_not_happen(f"Unexpected resolution: {m.resolution}") | ||
) | ||
) | ||
for m in resolved_markets | ||
] | ||
) | ||
st.markdown( | ||
f"Total number of open markets {len(open_markets)} and resolved markets {len(resolved_markets)}" | ||
"\n\n" | ||
f"Mean proportion of 'YES' in open markets: {all_open_markets_yes_mean:.2f}" | ||
"\n\n" | ||
f"Mean proportion of 'YES' in resolved markets: {all_resolved_markets_yes_mean:.2f}" | ||
) |
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.
The new monitor_market
function introduces comprehensive logic for analyzing and visualizing market data. Consider the following points for improvement:
- The usage of
groupby
requires the input list to be sorted by the key function (lambda x: x.created_time.date()
). Ensure thatopen_markets
andresolved_markets
are sorted before applyinggroupby
, or sort them within the function. - The handling of unexpected resolutions using
should_not_happen
is a good practice for error detection. However, ensure that this does not interrupt the user experience by catching exceptions and displaying an appropriate message instead. - The visualization with Altair charts is a great addition. Verify that the charts display correctly in the intended environment (e.g., Streamlit) and that the data is presented in an understandable manner.
Overall, the function significantly enhances the tool's capabilities but requires careful attention to error handling and data preparation.
+ # Ensure markets are sorted by created_time.date() before applying groupby
+ open_markets.sort(key=lambda x: x.created_time.date())
+ resolved_markets.sort(key=lambda x: x.created_time.date())
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.
def monitor_market(open_markets: list[Market], resolved_markets: list[Market]) -> None: | |
date_to_open_yes_proportion = { | |
d: np.mean([int(m.p_yes > 0.5) for m in markets]) | |
for d, markets in groupby(open_markets, lambda x: x.created_time.date()) | |
} | |
date_to_resolved_yes_proportion = { | |
d: np.mean( | |
[ | |
( | |
1 | |
if m.resolution == "YES" | |
else ( | |
0 | |
if m.resolution == "NO" | |
else should_not_happen(f"Unexpected resolution: {m.resolution}") | |
) | |
) | |
for m in markets | |
] | |
) | |
for d, markets in groupby(resolved_markets, lambda x: x.created_time.date()) | |
} | |
df_open = pd.DataFrame( | |
date_to_open_yes_proportion.items(), columns=["date", "open_proportion"] | |
) | |
df_open["open_label"] = "Open's yes proportion" | |
df_resolved = pd.DataFrame( | |
date_to_resolved_yes_proportion.items(), columns=["date", "resolved_proportion"] | |
) | |
df_resolved["resolved_label"] = "Resolved's yes proportion" | |
df = pd.merge(df_open, df_resolved, on="date") | |
open_chart = ( | |
alt.Chart(df) | |
.mark_line() | |
.encode(x="date:T", y="open_proportion:Q", color="open_label:N") | |
) | |
resolved_chart = ( | |
alt.Chart(df) | |
.mark_line() | |
.encode(x="date:T", y="resolved_proportion:Q", color="resolved_label:N") | |
) | |
st.altair_chart( | |
alt.layer(open_chart, resolved_chart).interactive(), # type: ignore # Doesn't expect `LayerChart`, but `Chart`, yet it works. | |
use_container_width=True, | |
) | |
all_open_markets_yes_mean = np.mean([int(m.p_yes > 0.5) for m in open_markets]) | |
all_resolved_markets_yes_mean = np.mean( | |
[ | |
( | |
1 | |
if m.resolution == "YES" | |
else ( | |
0 | |
if m.resolution == "NO" | |
else should_not_happen(f"Unexpected resolution: {m.resolution}") | |
) | |
) | |
for m in resolved_markets | |
] | |
) | |
st.markdown( | |
f"Total number of open markets {len(open_markets)} and resolved markets {len(resolved_markets)}" | |
"\n\n" | |
f"Mean proportion of 'YES' in open markets: {all_open_markets_yes_mean:.2f}" | |
"\n\n" | |
f"Mean proportion of 'YES' in resolved markets: {all_resolved_markets_yes_mean:.2f}" | |
) | |
def monitor_market(open_markets: list[Market], resolved_markets: list[Market]) -> None: | |
# Ensure markets are sorted by created_time.date() before applying groupby | |
open_markets.sort(key=lambda x: x.created_time.date()) | |
resolved_markets.sort(key=lambda x: x.created_time.date()) | |
date_to_open_yes_proportion = { | |
d: np.mean([int(m.p_yes > 0.5) for m in markets]) | |
for d, markets in groupby(open_markets, lambda x: x.created_time.date()) | |
} | |
date_to_resolved_yes_proportion = { | |
d: np.mean( | |
[ | |
( | |
1 | |
if m.resolution == "YES" | |
else ( | |
0 | |
if m.resolution == "NO" | |
else should_not_happen(f"Unexpected resolution: {m.resolution}") | |
) | |
) | |
for m in markets | |
] | |
) | |
for d, markets in groupby(resolved_markets, lambda x: x.created_time.date()) | |
} | |
df_open = pd.DataFrame( | |
date_to_open_yes_proportion.items(), columns=["date", "open_proportion"] | |
) | |
df_open["open_label"] = "Open's yes proportion" | |
df_resolved = pd.DataFrame( | |
date_to_resolved_yes_proportion.items(), columns=["date", "resolved_proportion"] | |
) | |
df_resolved["resolved_label"] = "Resolved's yes proportion" | |
df = pd.merge(df_open, df_resolved, on="date") | |
open_chart = ( | |
alt.Chart(df) | |
.mark_line() | |
.encode(x="date:T", y="open_proportion:Q", color="open_label:N") | |
) | |
resolved_chart = ( | |
alt.Chart(df) | |
.mark_line() | |
.encode(x="date:T", y="resolved_proportion:Q", color="resolved_label:N") | |
) | |
st.altair_chart( | |
alt.layer(open_chart, resolved_chart).interactive(), # type: ignore # Doesn't expect `LayerChart`, but `Chart`, yet it works. | |
use_container_width=True, | |
) | |
all_open_markets_yes_mean = np.mean([int(m.p_yes > 0.5) for m in open_markets]) | |
all_resolved_markets_yes_mean = np.mean( | |
[ | |
( | |
1 | |
if m.resolution == "YES" | |
else ( | |
0 | |
if m.resolution == "NO" | |
else should_not_happen(f"Unexpected resolution: {m.resolution}") | |
) | |
) | |
for m in resolved_markets | |
] | |
) | |
st.markdown( | |
f"Total number of open markets {len(open_markets)} and resolved markets {len(resolved_markets)}" | |
"\n\n" | |
f"Mean proportion of 'YES' in open markets: {all_open_markets_yes_mean:.2f}" | |
"\n\n" | |
f"Mean proportion of 'YES' in resolved markets: {all_resolved_markets_yes_mean:.2f}" | |
) |
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
|
||
|
||
def get_manifold_markets( | ||
number: int = 100, | ||
excluded_questions: t.List[str] = [], |
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.
Thanks
Summary by CodeRabbit
.gitignore
to include a new configuration file type.