Updating Metaculus models #483
Conversation
WalkthroughThe changes in this pull request involve significant restructuring of the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
prediction_market_agent_tooling/markets/metaculus/data_models.py (2)
6-6: Organize imports according to PEP 8 guidelinesTo enhance code readability and maintainability, organize the import statements following PEP 8 recommendations:
- Standard library imports
- Related third-party imports
- Local application/library-specific imports
Each group should be separated by a blank line.
Suggested reordering:
from enum import Enum +from typing import Any, Optional from pydantic import BaseModel +from prediction_market_agent_tooling.tools.utils import DatetimeUTC +from prediction_market_agent_tooling.gtypes import Probability -from typing import Any -from prediction_market_agent_tooling.gtypes import Probability -from prediction_market_agent_tooling.tools.utils import DatetimeUTC
60-63: Add a docstring to thepage_urlpropertyIncluding a docstring for the
page_urlproperty enhances code clarity by explaining the purpose and return value of the method.Suggested addition:
@property def page_url(self) -> str: + """Return the URL of the Metaculus question page.""" return f"https://www.metaculus.com/questions/{self.id}/"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- prediction_market_agent_tooling/markets/metaculus/data_models.py (1 hunks)
- prediction_market_agent_tooling/markets/metaculus/metaculus.py (1 hunks)
- tests/markets/test_markets.py (1 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/markets/metaculus/data_models.py
67-67: f-string without any placeholders
Remove extraneous
fprefix(F541)
🔇 Additional comments (1)
tests/markets/test_markets.py (1)
81-81: Increased limit for market retrieval in test case.The change from 10 to 100 for the
limitvariable allows for testing with a larger dataset. This can potentially uncover edge cases or issues that might not be apparent with a smaller sample size, which is beneficial for thorough testing.However, consider the following:
- This change might increase the test execution time, especially when run for multiple market types. Ensure that this doesn't significantly impact the overall test suite performance.
- It might be worth adding a comment explaining the rationale behind choosing 100 as the new limit.
To ensure this change doesn't significantly impact test execution time, you can run the following script:
This will provide information about the execution time of the
test_get_marketsfunction, which you can compare with previous runs to assess the impact of the change.
| current_p_yes=model.p_yes, | ||
| created_time=model.created_at, | ||
| close_time=model.scheduled_close_time, | ||
| url=model.page_url, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Inconsistent attribute usage with data_models.py
The from_data_model method references attributes (p_yes, scheduled_close_time, page_url, my_forecasts) that are not present in the MetaculusQuestion or Question classes within data_models.py. Please verify these attributes are correctly defined or update the method to align with the existing data models.
🔗 Analysis chain
Verify attribute assignments and update documentation
The changes in the from_data_model method reflect updates to the MetaculusQuestion model structure. Please ensure these modifications maintain the intended functionality:
- Confirm that
model.p_yesis the correct attribute and doesn't require theProbabilitywrapper. - Verify that
created_at,scheduled_close_time, andpage_urlare the correct attribute names in the updatedMetaculusQuestionmodel. - Check if the new logic for
have_predictedusingmodel.question.my_forecasts.latestis accurate.
Consider adding type hints to improve code clarity:
@staticmethod
def from_data_model(model: MetaculusQuestion) -> "MetaculusAgentMarket":
return MetaculusAgentMarket(
id=str(model.id),
question=model.title,
outcomes=[],
resolution=None,
current_p_yes=model.p_yes, # type: Probability
created_time=model.created_at, # type: DatetimeUTC
close_time=model.scheduled_close_time, # type: DatetimeUTC
url=model.page_url, # type: str
volume=None,
have_predicted=model.question.my_forecasts.latest is not None, # type: bool
outcome_token_pool=None,
)Update the method's docstring to reflect these changes and explain the new attribute mappings.
To ensure consistency with the data_models.py file, run the following script:
Also applies to: 45-45
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the attribute names in the MetaculusQuestion model
# Test: Check for the presence of new attribute names
rg --type python 'class MetaculusQuestion.*?:.*?(p_yes|created_at|scheduled_close_time|page_url)' prediction_market_agent_tooling/markets/metaculus/data_models.py
# Test: Check for the presence of my_forecasts attribute
rg --type python 'class Question.*?:.*?my_forecasts' prediction_market_agent_tooling/markets/metaculus/data_models.py
Length of output: 346
Script:
#!/bin/bash
# Description: Verify the attribute names in the MetaculusQuestion model
# Test: Check for the presence of new attribute names in MetaculusQuestion
rg -g "*.py" 'class\s+MetaculusQuestion.*?\n(?:.*\n)*?.*\b(p_yes|created_at|scheduled_close_time|page_url)\b' prediction_market_agent_tooling/markets/metaculus/data_models.py
# Test: Check for the presence of my_forecasts attribute in Question class
rg -g "*.py" 'class\s+Question.*?\n(?:.*\n)*?.*\bmy_forecasts\b' prediction_market_agent_tooling/markets/metaculus/data_models.py
Length of output: 692
Script:
#!/bin/bash
# Description: Verify the attribute names in the MetaculusQuestion model with multiline support
# Test: Check for the presence of new attribute names in MetaculusQuestion
rg -U -g "*.py" 'class\s+MetaculusQuestion.*?\n(?:.*\n)*?.*\b(p_yes|created_at|scheduled_close_time|page_url)\b' prediction_market_agent_tooling/markets/metaculus/data_models.py
# Test: Check for the presence of my_forecasts attribute in Question class
rg -U -g "*.py" 'class\s+Question.*?\n(?:.*\n)*?.*\bmy_forecasts\b' prediction_market_agent_tooling/markets/metaculus/data_models.py
Length of output: 1245
| @property | ||
| def p_yes(self) -> Probability: | ||
| if self.question.type != QuestionType.binary: | ||
| raise ValueError(f"Only binary markets can have p_yes.") |
There was a problem hiding this comment.
Remove unnecessary f prefix in exception message
At line 67, the ValueError is using an f-string without any placeholders. The f prefix is unnecessary and can be removed to clean up the code.
Apply this change:
- raise ValueError(f"Only binary markets can have p_yes.")
+ raise ValueError("Only binary markets can have p_yes.")📝 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.
| raise ValueError(f"Only binary markets can have p_yes.") | |
| raise ValueError("Only binary markets can have p_yes.") |
🧰 Tools
🪛 Ruff
67-67: f-string without any placeholders
Remove extraneous
fprefix(F541)
| group = "group" | ||
| conditional_group = "conditional_group" | ||
| multiple_choice = "multiple_choice" | ||
| binary = "binary" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying QuestionType Enum with only one member
The QuestionType enum now contains only a single member binary. Enums are typically used to define a set of constant values. If no other question types are expected, it might be more appropriate to replace the enum with a simple constant or a type alias to reduce unnecessary complexity.
Suggested change:
-from enum import Enum
+from typing import Literal
-class QuestionType(str, Enum):
- binary = "binary"
+QuestionType = Literal["binary"]📝 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.
| binary = "binary" | |
| from typing import Literal | |
| QuestionType = Literal["binary"] |
| class AggregationItem(BaseModel): | ||
| start_time: DatetimeUTC | ||
| end_time: DatetimeUTC | None | ||
| forecast_values: list[float] | None | ||
| forecaster_count: int | ||
| interval_lower_bounds: list[float] | None | ||
| centers: list[float] | None | ||
| interval_upper_bounds: list[float] | None | ||
| means: list[float] | None | ||
| histogram: list[float] | None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Optional from typing for optional type hints
In the AggregationItem class, several fields are annotated with the union syntax | None to indicate that they can be None. For better compatibility with earlier Python versions (prior to 3.10) and to improve readability, consider importing Optional from the typing module.
Apply the following changes:
-from typing import Any
+from typing import Any, Optional
class AggregationItem(BaseModel):
start_time: DatetimeUTC
- end_time: DatetimeUTC | None
- forecast_values: list[float] | None
+ end_time: Optional[DatetimeUTC]
+ forecast_values: Optional[list[float]]
forecaster_count: int
- interval_lower_bounds: list[float] | None
- centers: list[float] | None
- interval_upper_bounds: list[float] | None
- means: list[float] | None
- histogram: list[float] | None
+ interval_lower_bounds: Optional[list[float]]
+ centers: Optional[list[float]]
+ interval_upper_bounds: Optional[list[float]]
+ means: Optional[list[float]]
+ histogram: Optional[list[float]]📝 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.
| class AggregationItem(BaseModel): | |
| start_time: DatetimeUTC | |
| end_time: DatetimeUTC | None | |
| forecast_values: list[float] | None | |
| forecaster_count: int | |
| interval_lower_bounds: list[float] | None | |
| centers: list[float] | None | |
| interval_upper_bounds: list[float] | None | |
| means: list[float] | None | |
| histogram: list[float] | None | |
| from typing import Any, Optional | |
| class AggregationItem(BaseModel): | |
| start_time: DatetimeUTC | |
| end_time: Optional[DatetimeUTC] | |
| forecast_values: Optional[list[float]] | |
| forecaster_count: int | |
| interval_lower_bounds: Optional[list[float]] | |
| centers: Optional[list[float]] | |
| interval_upper_bounds: Optional[list[float]] | |
| means: Optional[list[float]] | |
| histogram: Optional[list[float]] |
| def p_yes(self) -> Probability: | ||
| if self.question.type != QuestionType.binary: | ||
| raise ValueError(f"Only binary markets can have p_yes.") | ||
| if ( | ||
| self.question.aggregations.recency_weighted is None | ||
| or self.question.aggregations.recency_weighted.latest is None | ||
| or self.question.aggregations.recency_weighted.latest.forecast_values | ||
| is None | ||
| ): | ||
| # If no value is provided (i.e. the question is new and has not been answered yet), we default to 0.5. | ||
| return Probability(0.5) | ||
| if len(self.question.aggregations.recency_weighted.latest.forecast_values) != 2: | ||
| raise ValueError( | ||
| f"Invalid logic, assumed that binary markets will have two forecasts, got: {self.question.aggregations.recency_weighted.latest.forecast_values}" | ||
| ) | ||
| # Experimentally figured out that they store "Yes" at index 1. | ||
| return Probability( | ||
| self.question.aggregations.recency_weighted.latest.forecast_values[1] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle non-binary question types gracefully in p_yes property
Currently, the p_yes property raises a ValueError if the question type is not binary. To make the property safer and more user-friendly, consider returning None or a default value when the question type is not binary. This avoids exceptions and allows calling code to handle non-binary questions appropriately.
Suggested refactor:
def p_yes(self) -> Probability | None:
- if self.question.type != QuestionType.binary:
- raise ValueError("Only binary markets can have p_yes.")
+ if self.question.type != QuestionType.binary:
+ return None
if (
self.question.aggregations.recency_weighted is NoneAdditionally, update the return type annotation to reflect that None can be returned:
- def p_yes(self) -> Probability:
+ def p_yes(self) -> Probability | None:📝 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 p_yes(self) -> Probability: | |
| if self.question.type != QuestionType.binary: | |
| raise ValueError(f"Only binary markets can have p_yes.") | |
| if ( | |
| self.question.aggregations.recency_weighted is None | |
| or self.question.aggregations.recency_weighted.latest is None | |
| or self.question.aggregations.recency_weighted.latest.forecast_values | |
| is None | |
| ): | |
| # If no value is provided (i.e. the question is new and has not been answered yet), we default to 0.5. | |
| return Probability(0.5) | |
| if len(self.question.aggregations.recency_weighted.latest.forecast_values) != 2: | |
| raise ValueError( | |
| f"Invalid logic, assumed that binary markets will have two forecasts, got: {self.question.aggregations.recency_weighted.latest.forecast_values}" | |
| ) | |
| # Experimentally figured out that they store "Yes" at index 1. | |
| return Probability( | |
| self.question.aggregations.recency_weighted.latest.forecast_values[1] | |
| ) | |
| def p_yes(self) -> Probability | None: | |
| if self.question.type != QuestionType.binary: | |
| return None | |
| if ( | |
| self.question.aggregations.recency_weighted is None | |
| or self.question.aggregations.recency_weighted.latest is None | |
| or self.question.aggregations.recency_weighted.latest.forecast_values | |
| is None | |
| ): | |
| # If no value is provided (i.e. the question is new and has not been answered yet), we default to 0.5. | |
| return Probability(0.5) | |
| if len(self.question.aggregations.recency_weighted.latest.forecast_values) != 2: | |
| raise ValueError( | |
| f"Invalid logic, assumed that binary markets will have two forecasts, got: {self.question.aggregations.recency_weighted.latest.forecast_values}" | |
| ) | |
| # Experimentally figured out that they store "Yes" at index 1. | |
| return Probability( | |
| self.question.aggregations.recency_weighted.latest.forecast_values[1] | |
| ) |
🧰 Tools
🪛 Ruff
67-67: f-string without any placeholders
Remove extraneous
fprefix(F541)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- prediction_market_agent_tooling/markets/metaculus/data_models.py (1 hunks)
🧰 Additional context used
🪛 Ruff
prediction_market_agent_tooling/markets/metaculus/data_models.py
82-82: f-string without any placeholders
Remove extraneous
fprefix(F541)
| if self.question.type != QuestionType.binary: | ||
| raise ValueError(f"Only binary markets can have p_yes.") | ||
| if ( | ||
| self.question.aggregations.recency_weighted is None | ||
| or self.question.aggregations.recency_weighted.latest is None | ||
| or self.question.aggregations.recency_weighted.latest.forecast_values | ||
| is None | ||
| ): | ||
| # If no value is provided (i.e. the question is new and has not been answered yet), we default to 0.5. | ||
| return Probability(0.5) | ||
| if len(self.question.aggregations.recency_weighted.latest.forecast_values) != 2: | ||
| raise ValueError( | ||
| f"Invalid logic, assumed that binary markets will have two forecasts, got: {self.question.aggregations.recency_weighted.latest.forecast_values}" | ||
| ) | ||
| # Experimentally figured out that they store "Yes" at index 1. | ||
| return Probability( | ||
| self.question.aggregations.recency_weighted.latest.forecast_values[1] | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the p_yes property more robust to data format changes
In the p_yes property, the index 1 is hardcoded with the assumption that "Yes" is stored at that position in forecast_values. This reliance on a specific index may lead to errors if Metaculus changes their data format in the future. To improve robustness, consider identifying the correct index dynamically or using a key-value mapping if available.
🧰 Tools
🪛 Ruff
82-82: f-string without any placeholders
Remove extraneous
fprefix(F541)
| raise ValueError( | ||
| f"Invalid logic, assumed that binary markets will have two forecasts, got: {self.question.aggregations.recency_weighted.latest.forecast_values}" | ||
| ) | ||
| # Experimentally figured out that they store "Yes" at index 1. |
There was a problem hiding this comment.
😱 why so complicated metaculuuuuuus!!
Based on https://www.metaculus.com/notebooks/28595/updates-to-metaculus-api/ and my observations from their responses.