-
Notifications
You must be signed in to change notification settings - Fork 190
feat examples crypto: Add and refine example notebooks #589
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
Conversation
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.
Overall LGTM! Left small comments.
examples/crypto/README.md
Outdated
|
||
| Notebook | Open in Google Colab | | ||
|:----------------------------------------------|:------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------:| | ||
| [BTC/USD Swing Trade](crypto-btc-usd-swing-trade.ipynb) | [](https://colab.research.google.com/github/alpacahq/alpaca-py/blob/master/examples/crypto/crypto-btc-usd-swing-trade.ipynb.ipynb)| |
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.
typo
| [BTC/USD Swing Trade](crypto-btc-usd-swing-trade.ipynb) | [](https://colab.research.google.com/github/alpacahq/alpaca-py/blob/master/examples/crypto/crypto-btc-usd-swing-trade.ipynb.ipynb)| | |
| [BTC/USD Swing Trade](crypto-btc-usd-swing-trade.ipynb) | [](https://colab.research.google.com/github/alpacahq/alpaca-py/blob/master/examples/crypto/crypto-btc-usd-swing-trade.ipynb)| |
@@ -11,7 +11,7 @@ | |||
"cell_type": "markdown", | |||
"metadata": {}, | |||
"source": [ | |||
"[](https://colab.research.google.com/github/alpacahq/alpaca-py/blob/master/examples/crypto-trading-basic.ipynb)" | |||
"[](https://colab.research.google.com/github/alpacahq/alpaca-py/blob/master/examples/crypto/crypto-trading-basic.ipynb)" |
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.
Oh good catch! Could you please also update stocks-trading-basic.ipynb
/ options-trading-basic.ipynb
/ options-trading-mleg.ipynb
?
"import pandas as pd\n", | ||
"import pandas_ta as ta # The installment of pandas-ta may be tricky. Please find the pypi page here (https://pypi.org/project/pandas-ta/)\n", | ||
"import numpy as np\n", | ||
"import matplotlib.pyplot as plt\n", | ||
"from datetime import datetime, timedelta\n", | ||
"from zoneinfo import ZoneInfo\n", | ||
"\n", | ||
"import alpaca\n", | ||
"from alpaca.data.timeframe import TimeFrame, TimeFrameUnit\n", | ||
"from alpaca.data.historical.crypto import CryptoHistoricalDataClient\n", | ||
"\n", | ||
"from alpaca.data.requests import (\n", | ||
" CryptoBarsRequest,\n", | ||
" CryptoQuoteRequest,\n", | ||
" CryptoTradesRequest,\n", | ||
" CryptoLatestQuoteRequest\n", | ||
" )\n", | ||
"from alpaca.trading.client import TradingClient\n", | ||
"from alpaca.trading.requests import MarketOrderRequest, LimitOrderRequest\n", | ||
"from alpaca.trading.enums import (\n", | ||
" AssetClass,\n", | ||
" AssetStatus,\n", | ||
" OrderSide,\n", | ||
" OrderType,\n", | ||
" TimeInForce,\n", | ||
" QueryOrderStatus\n", | ||
")" |
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.
minor: How about applying isort?
"df = bars_date.copy()\n", | ||
"df.head()" |
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.
How about adding a comment to explain the reason of using .copy()
here?
In my understanding, if we copy here, we do not need to fetch data (i.e. bars_date) again within same session. It might make easy to change and check below codes if user want to tweak.
"metadata": {}, | ||
"outputs": [], | ||
"source": [ | ||
"# Define EMA periods\n", |
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.
How about adding explanation of abbreviation for EMA
similar to SMA
/ ATR
/ ADX
?
"metadata": {}, | ||
"outputs": [], | ||
"source": [ | ||
"# Get the latest price of the underlying stock\n", |
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.
"# Get the latest price of the underlying stock\n", | |
"# Get the estimated buying cost of BTC/USD with given qty\n", |
…wing-trade.ipynb' per review
Updated the notebooks and README.md per review |
Focus review on commit
f454485,
9ad7391,
anda076992,
This pull request adds and refines several example notebooks in the examples/crypto directory: