-
Notifications
You must be signed in to change notification settings - Fork 152
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
Fix get_historical_prices for PolygonDataBacktesting data source #433
Conversation
My review is in progress 📖 - I will have feedback for you in a few minutes! |
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 have reviewed your code and found 2 potential issues. To discuss my individual comments that I have added, tag me in replies using @korbit-ai.
Please react with a 👍 to my comments that you find helpful and a 👎 to those you find unhelpful - this will help me learn and improve as we collaborate.
def test_get_historical_prices_is_not_none(self): | ||
tzinfo = pytz.timezone("America/New_York") | ||
start = datetime.datetime(2024, 2, 5).astimezone(tzinfo) | ||
end = datetime.datetime(2024, 2, 10).astimezone(tzinfo) | ||
|
||
data_source = PolygonDataBacktesting( | ||
start, end, api_key=POLYGON_API_KEY, has_paid_subscription=True | ||
) | ||
data_source._datetime = datetime.datetime(2024, 2, 7, 10).astimezone(tzinfo) | ||
prices = data_source.get_historical_prices("SPY", 1, "day") | ||
assert prices is not 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.
The test case test_get_historical_prices_is_not_none
is not comprehensive enough. It only checks if the get_historical_prices
function returns a non-None value. It would be better to also check if the returned value is as expected, for example, by comparing it to a known correct value or checking its type and structure.
@@ -51,7 +51,7 @@ def _enforce_storage_limit(pandas_data: OrderedDict): | |||
storage_used -= mu | |||
logging.info(f"Storage limit exceeded. Evicted LRU data: {k} used {mu:,} bytes") | |||
|
|||
def _update_pandas_data(self, asset, quote, length, timestep, start_dt=None, update_data_store=False): | |||
def _update_pandas_data(self, asset, quote, length, timestep, start_dt=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.
The removal of the update_data_store
argument from the _update_pandas_data
function could potentially lead to issues if there are other parts of the code that still expect this argument to be present. It would be a good idea to search the entire codebase for any calls to _update_pandas_data
and ensure that they are updated to match the new function signature.
Note:
|
At head,
get_historical_prices
always returns None for Polygon data source. The reason is thatget_historical_prices
internally calls _pull_source_symbol_bars, which tries to retrieve the data fromself._data_store
. Without updating the data store, above script will always return None.Per discussion in #430, this PR merges data_store and pandas_data. It also removes the
update_data_store
argument and simplifies the code logic.