feat(analysis): add technical indicators module#41
Merged
Conversation
Add `client.analysis` (AnalysisResource) with SMA, EMA, RSI, MACD, Bollinger Bands, and ATR. Supports both the DataFrame helper (`with_indicators(df, indicators=[...])`, non-mutating) and direct per-Series calculation. Pure pandas/numpy implementation with no new hard dependencies; pandas remains the optional `[pandas]` extra and is guarded with the same friendly ImportError used elsewhere in the SDK. Includes full unit coverage (12 tests, analysis.py at ~87%), README documentation with examples for both usage styles, and a CHANGELOG entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new ChangesTechnical Indicators Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant OilPriceAPI
participant AnalysisResource
participant PandasSeries
User->>OilPriceAPI: client.analysis.with_indicators(df, ["sma_20", "rsi", "bb"])
OilPriceAPI->>AnalysisResource: with_indicators(df, indicators, column="value")
AnalysisResource->>AnalysisResource: _require_pandas()
AnalysisResource->>AnalysisResource: validate column exists in df
loop each indicator token
AnalysisResource->>AnalysisResource: _parse_period(token, default)
AnalysisResource->>PandasSeries: sma / rsi / bollinger_bands(series, period)
PandasSeries-->>AnalysisResource: computed Series or DataFrame columns
AnalysisResource->>AnalysisResource: append columns to copied df
end
AnalysisResource-->>User: df copy with indicator columns added
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
66-95: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify ATR availability in with_indicators() vs. direct methods.
The supported indicators list (line 95) includes ATR alongside the others, but the examples only show ATR being used as a direct method (
client.analysis.atr(high, low, close, period=14)). ATR cannot be used viawith_indicators()because it requires three input Series (high, low, close) rather than a single price column.Consider adding a note to prevent users from trying
indicators=["atr"]in thewith_indicators()call, which would raise aValueError.💡 Example clarification to add after the Method 2 section
**Note:** ATR requires three inputs (high, low, close prices) and is only available as a direct method. The `with_indicators()` DataFrame helper supports SMA, EMA, RSI, MACD, and Bollinger Bands.Alternatively, add a comment to the indicator list to clarify the distinction:
Supported indicators in `with_indicators()`: SMA, EMA, RSI, MACD, Bollinger Bands. Additional direct methods: ATR (requires high/low/close inputs).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 66 - 95, The supported indicators list at line 95 includes ATR alongside other indicators without clarifying that ATR cannot be used with the with_indicators() method. Since ATR requires three separate inputs (high, low, close prices) unlike the other indicators which work with a single price column, add a clarifying note after the Method 2 section to explicitly state that ATR is only available as a direct method (client.analysis.atr) and that with_indicators() only supports SMA, EMA, RSI, MACD, and Bollinger Bands to prevent users from attempting to use indicators=["atr"] which would raise a ValueError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@oilpriceapi/resources/analysis.py`:
- Around line 231-250: The issue is that RSI, SMA, and EMA indicators are being
stored in the output dictionary using the raw indicator token as the key instead
of the normalized name. For consistency with Bollinger Bands and MACD, and to
avoid duplicate/unstable schema columns, replace all instances of out[indicator]
with out[name] in the conditional blocks for RSI, SMA, and EMA. The variable
name is already normalized via strip() and lower() at the beginning of the loop,
so using this normalized name ensures consistent, canonical column names
regardless of the case or whitespace in the input token.
In `@tests/unit/test_analysis_resource.py`:
- Around line 100-109: The test_with_indicators_adds_requested_columns test
verifies that MACD columns are included in the output when the MACD indicator is
requested, but it only checks for the macd and macd_signal columns. Add an
additional assertion to also verify that macd_histogram is present in
out.columns, following the same pattern as the existing assertions for macd and
macd_signal, so that any regression removing the macd_histogram output would be
caught by this test.
---
Nitpick comments:
In `@README.md`:
- Around line 66-95: The supported indicators list at line 95 includes ATR
alongside other indicators without clarifying that ATR cannot be used with the
with_indicators() method. Since ATR requires three separate inputs (high, low,
close prices) unlike the other indicators which work with a single price column,
add a clarifying note after the Method 2 section to explicitly state that ATR is
only available as a direct method (client.analysis.atr) and that
with_indicators() only supports SMA, EMA, RSI, MACD, and Bollinger Bands to
prevent users from attempting to use indicators=["atr"] which would raise a
ValueError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 528b806b-fa3d-40a6-8288-b3d52e2d3ddb
📒 Files selected for processing (6)
CHANGELOG.mdREADME.mdoilpriceapi/client.pyoilpriceapi/resources/__init__.pyoilpriceapi/resources/analysis.pytests/unit/test_analysis_resource.py
Comment on lines
+231
to
+250
| for indicator in indicators: | ||
| name = indicator.strip().lower() | ||
|
|
||
| if name in ("bollinger_bands", "bollinger", "bb"): | ||
| bands = self.bollinger_bands(prices) | ||
| for col in bands.columns: | ||
| out[col] = bands[col] | ||
| elif name == "macd": | ||
| macd_df = self.macd(prices) | ||
| for col in macd_df.columns: | ||
| out[col] = macd_df[col] | ||
| elif name in ("rsi",) or name.startswith("rsi_"): | ||
| period = self._parse_period(name, default=14) | ||
| out[indicator] = self.rsi(prices, period=period) | ||
| elif name == "sma" or name.startswith("sma_"): | ||
| period = self._parse_period(name, default=20) | ||
| out[indicator] = self.sma(prices, period=period) | ||
| elif name == "ema" or name.startswith("ema_"): | ||
| period = self._parse_period(name, default=20) | ||
| out[indicator] = self.ema(prices, period=period) |
There was a problem hiding this comment.
Normalize indicator-derived output column names.
with_indicators() parses indicator tokens case/whitespace-insensitively, but SMA/EMA/RSI columns are written using the raw token. This produces unstable schemas (e.g., " SMA_20 " or duplicate "SMA_20"/"sma_20" columns) instead of canonical names.
💡 Proposed fix
for indicator in indicators:
name = indicator.strip().lower()
if name in ("bollinger_bands", "bollinger", "bb"):
bands = self.bollinger_bands(prices)
for col in bands.columns:
out[col] = bands[col]
elif name == "macd":
macd_df = self.macd(prices)
for col in macd_df.columns:
out[col] = macd_df[col]
elif name in ("rsi",) or name.startswith("rsi_"):
period = self._parse_period(name, default=14)
- out[indicator] = self.rsi(prices, period=period)
+ out[name] = self.rsi(prices, period=period)
elif name == "sma" or name.startswith("sma_"):
period = self._parse_period(name, default=20)
- out[indicator] = self.sma(prices, period=period)
+ out[name] = self.sma(prices, period=period)
elif name == "ema" or name.startswith("ema_"):
period = self._parse_period(name, default=20)
- out[indicator] = self.ema(prices, period=period)
+ out[name] = self.ema(prices, period=period)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@oilpriceapi/resources/analysis.py` around lines 231 - 250, The issue is that
RSI, SMA, and EMA indicators are being stored in the output dictionary using the
raw indicator token as the key instead of the normalized name. For consistency
with Bollinger Bands and MACD, and to avoid duplicate/unstable schema columns,
replace all instances of out[indicator] with out[name] in the conditional blocks
for RSI, SMA, and EMA. The variable name is already normalized via strip() and
lower() at the beginning of the loop, so using this normalized name ensures
consistent, canonical column names regardless of the case or whitespace in the
input token.
Comment on lines
+100
to
+109
| def test_with_indicators_adds_requested_columns(self, client, price_df): | ||
| out = client.analysis.with_indicators( | ||
| price_df, | ||
| indicators=["sma_20", "sma_50", "rsi", "bollinger_bands", "macd"], | ||
| ) | ||
| for col in ["sma_20", "sma_50", "rsi", "bb_upper", "bb_middle", "bb_lower"]: | ||
| assert col in out.columns, f"missing column {col}" | ||
| assert "macd" in out.columns | ||
| assert "macd_signal" in out.columns | ||
|
|
There was a problem hiding this comment.
Assert macd_histogram in the helper output contract test.
This test checks only two MACD columns, so a regression removing macd_histogram would not be caught.
✅ Suggested test addition
for col in ["sma_20", "sma_50", "rsi", "bb_upper", "bb_middle", "bb_lower"]:
assert col in out.columns, f"missing column {col}"
assert "macd" in out.columns
assert "macd_signal" in out.columns
+ assert "macd_histogram" in out.columns📝 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.
Suggested change
| def test_with_indicators_adds_requested_columns(self, client, price_df): | |
| out = client.analysis.with_indicators( | |
| price_df, | |
| indicators=["sma_20", "sma_50", "rsi", "bollinger_bands", "macd"], | |
| ) | |
| for col in ["sma_20", "sma_50", "rsi", "bb_upper", "bb_middle", "bb_lower"]: | |
| assert col in out.columns, f"missing column {col}" | |
| assert "macd" in out.columns | |
| assert "macd_signal" in out.columns | |
| def test_with_indicators_adds_requested_columns(self, client, price_df): | |
| out = client.analysis.with_indicators( | |
| price_df, | |
| indicators=["sma_20", "sma_50", "rsi", "bollinger_bands", "macd"], | |
| ) | |
| for col in ["sma_20", "sma_50", "rsi", "bb_upper", "bb_middle", "bb_lower"]: | |
| assert col in out.columns, f"missing column {col}" | |
| assert "macd" in out.columns | |
| assert "macd_signal" in out.columns | |
| assert "macd_histogram" in out.columns |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/test_analysis_resource.py` around lines 100 - 109, The
test_with_indicators_adds_requested_columns test verifies that MACD columns are
included in the output when the MACD indicator is requested, but it only checks
for the macd and macd_signal columns. Add an additional assertion to also verify
that macd_histogram is present in out.columns, following the same pattern as the
existing assertions for macd and macd_signal, so that any regression removing
the macd_histogram output would be caught by this test.
karlwaldman
added a commit
that referenced
this pull request
Jul 3, 2026
Add `client.analysis` (AnalysisResource) with SMA, EMA, RSI, MACD, Bollinger Bands, and ATR. Supports both the DataFrame helper (`with_indicators(df, indicators=[...])`, non-mutating) and direct per-Series calculation. Pure pandas/numpy implementation with no new hard dependencies; pandas remains the optional `[pandas]` extra and is guarded with the same friendly ImportError used elsewhere in the SDK. Includes full unit coverage (12 tests, analysis.py at ~87%), README documentation with examples for both usage styles, and a CHANGELOG entry. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3
What
Adds a Technical Indicators module exposed as
client.analysis(AnalysisResource), implementing the indicators requested in the issue with a pure pandas/numpy implementation (no new hard dependencies).Indicators
bb_upper/bb_middle/bb_lower)Two usage styles (both from the issue)
pandasstays the optional[pandas]extra and is guarded with the same friendlyImportErrorused elsewhere in the SDK.TDD evidence (strict red-green-refactor)
RED — wrote
tests/unit/test_analysis_resource.pyfirst and confirmed it fails on unmodified code:GREEN — after adding
AnalysisResourceand wiring it onto the client:Full gate (mirrors
.github/workflows/test.yml):ruff check oilpriceapi/→ All checks passedmypy oilpriceapi/ --ignore-missing-imports→ Success: no issues found in 44 source files.[dev]only):326 passed, 10 skipped(the new pandas-gated test skips when pandas is absent, matching the existingto_dataframetest pattern), coverage 58.17% ≥ 50% gate347 passed,analysis.pyat 86.67% coverage, total 61.27%Scope / safety
Success criteria checklist
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests