Verified functionality of thetadata package #489
Conversation
|
My review is in progress 📖 - I will have feedback for you in a few minutes! |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have reviewed your code and found 4 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.
|
|
||
| # fig = go.Figure() | ||
| fig = make_subplots(specs=[[{"secondary_y": True}]]) | ||
|
|
||
| # Strategy line | ||
| # Updated format_positions function to handle lists and dicts | ||
| def format_positions(positions): |
There was a problem hiding this comment.
The format_positions function is currently nested within plot_returns, which can make the code less readable and maintainable. Consider defining format_positions at the module level to improve code organization and potential reusability.
| assert current_asset_price == 133.52 | ||
|
|
||
| # Option Chain: Get Full Option Chain Information | ||
| chain = self.get_chain(self.chains, exchange="SMART") | ||
| expiration = self.select_option_expiration(chain, days_to_expiration=1) | ||
| # expiration = datetime.date(2023, 8, 4) | ||
|
|
||
| strike_price = round(current_asset_price) | ||
| option_asset = Asset( | ||
| symbol=underlying_asset.symbol, | ||
| asset_type="option", | ||
| expiration=expiration, | ||
| right="CALL", | ||
| strike=strike_price, | ||
| multiplier=100, | ||
| ) | ||
| current_option_price = self.get_last_price(option_asset) | ||
|
|
||
| assert current_option_price == 4.10 | ||
|
|
||
| # Get historical prices for the option | ||
| option_prices = self.get_historical_prices(option_asset, 100, "minute") | ||
| df = option_prices.df | ||
|
|
||
| # Assert that the first price is the right price | ||
| assert df["close"].iloc[-1] == 4.11 | ||
|
|
||
| # Check that the time of the last bar is 2023-07-31T19:58:00.000Z | ||
| last_dt = df.index[-1] | ||
| assert last_dt == datetime.datetime(2023, 7, 31, 19, 58, tzinfo=datetime.timezone.utc) |
There was a problem hiding this comment.
The tests contain hardcoded values for asset prices and option prices, which can make the tests fragile and less reliable if the underlying data changes. Consider replacing these hardcoded values with mock objects or setup methods that generate the necessary data dynamically to improve test robustness.
lumibot/strategies/_strategy.py
Outdated
| import pandas as pd | ||
|
|
||
| from lumibot.backtesting import BacktestingBroker, PolygonDataBacktesting |
There was a problem hiding this comment.
Hi, I noticed that there's an inconsistency in the use of blank lines in the import section and between methods. Could you please align this with our team's style guide? Either we keep a single blank line after imports and between methods for clarity, or we remove unnecessary blank lines if compactness is preferred.
| data_source = ThetaDataBacktesting( | ||
| datetime_start=backtesting_start, | ||
| datetime_end=backtesting_end, | ||
| username=THETADATA_USERNAME, | ||
| password=THETADATA_PASSWORD, | ||
| ) |
There was a problem hiding this comment.
The test_thetadata_restclient method is making real API calls, which can cause flakiness in the tests. To improve test reliability, consider mocking these API calls to ensure that the tests can run in isolation without depending on external services.
haochili
force-pushed
the
thetadata_haochi
branch
from
de71d49 to
2745b1a
Compare
haochili
force-pushed
the
thetadata_haochi
branch
from
2745b1a to
77a116c
Compare
…utomatically kill the thetaTerminal.jar process before and after the unit test
Signed-off-by: Haochi Li <[email protected]>
…/password first. If not found, then try to find from local '.env' file Signed-off-by: Haochi Li <[email protected]>
…already in lumibot __init_.py The test script will exit with raise if it doesn't find credentials from environment variables username/password Signed-off-by: Haochi Li <[email protected]>
haochili
force-pushed
the
thetadata_haochi
branch
from
dbad89e to
4d6ccfd
Compare
…nch. The current test_polygon.py on this branch was outdated, and somehow didn't get synced with manual conflicts resolution. Signed-off-by: Haochi Li <[email protected]>
haochili
force-pushed
the
thetadata_haochi
branch
from
8ba5f24 to
1e30e3f
Compare
…e used for condor martingale and other strategies Still pending on skipping holidays and weekends features to remove the warnings. Signed-off-by: Haochi Li <[email protected]>
1. Introduced a automatic holiday + weekends detection feature in thetadata_helper.py When see weekends or holiday, skip streaming data from thetadata 2. Handled error messages, in skip days, avoid print out any error messages 3. If there is data stream error in normal days, still print out errors as original logic does Signed-off-by: Haochi Li <[email protected]>
…t, subtracted 1 min from thetadata
dummy change Signed-off-by: Haochi Li <[email protected]>
haochili
force-pushed
the
thetadata_haochi
branch
from
cb1dfac to
05626f8
Compare
I operated unit testing script that was created on thetadata package, verified unit test with passing result.
-Verified stock dataset can be downloaded from thetadata web to local father, then parsed by lumibot properly
-I manually run martingale_8 with this branch locally built on my computer, and verified existing backtesting feature (with polygon) wasn't broken. I got the same return as I did with "dev" branch: 3.5.12.
I added auto kill all ThetaTerminal.jar processes before and after the backtest. The original test script didn't have this step, causing the test cases to fail due to redundant process hanging from last round of test.
I changed the script to automatically search for .env file at the root dir of the lumibot repo. I placed the credentials like username/password in the .env file, so the test_thetadata.py can run.