feat: Remove Scanner class #54
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
- Coverage 71.12% 69.60% -1.53%
==========================================
Files 30 29 -1
Lines 2691 2586 -105
==========================================
- Hits 1914 1800 -114
- Misses 777 786 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Chocksy
force-pushed
the
feat/deprecate_scanner
branch
from
4ed97a1 to
8f7f3d4
Compare
WalkthroughThe changes in this pull request involve updates to multiple files, including modifications to the Python version, significant restructuring of classes and methods in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Base
participant DataHandler
participant FutureSpread
User->>Base: Call update()
Base->>Base: Check if market is closed
Base->>DataHandler: Get option contracts
DataHandler-->>Base: Return contracts
Base->>Base: Generate insights
Base->>FutureSpread: Update wing sizes
FutureSpread-->>Base: Confirm update
Base-->>User: Return results
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 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 full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (7)
Alpha/FutureSpread.py (2)
Line range hint
31-32: Address TODO comment regarding maxOrderQuantityThere's an unresolved TODO comment about fixing maxOrderQuantity based on buying power. This should be addressed to ensure proper position sizing.
Would you like me to help implement the buying power-based maxOrderQuantity calculation?
🧰 Tools
🪛 Ruff (0.8.0)
50-50:
timemay be undefined, or defined from star imports(F405)
Line range hint
89-91: Consider moving trade times to configurationThe trade times are currently hardcoded in the getOrder method. Consider moving these to the PARAMETERS dictionary for better configurability and maintenance.
PARAMETERS = { "scheduleStartTime": time(9, 30, 0), "scheduleStopTime": time(16, 0, 0), + "tradeTimes": [time(9, 35, 0), time(9, 40, 0), time(9, 45, 0)], ... }🧰 Tools
🪛 Ruff (0.8.0)
50-50:
timemay be undefined, or defined from star imports(F405)
Order/Base.py (3)
38-40: Add logging when position limits prevent order creationWhile checking position limits before order creation is important, consider adding a log statement to inform when an order is not created due to position limits. This will aid in debugging and monitoring system behavior.
Apply this diff to include a log message:
def buildOrderPosition(self, order, lastClosedOrderTag=None): # ... existing code ... # Check position limits first if not self.check_position_limits(order): + self.logger.info("Order not created: Position limits reached due to position limits.") return [None, None] # ... existing code ...
232-234: Simplify nestedifstatements incheck_position_limitsThe nested
ifstatements can be combined into a single condition for improved readability.Apply this diff to simplify the code:
- if self.strategy.parameter("checkForDuplicatePositions", True): - if self.hasDuplicateLegs(order): + if self.strategy.parameter("checkForDuplicatePositions", True) and self.hasDuplicateLegs(order): return False🧰 Tools
🪛 Ruff (0.8.0)
232-233: Use a single
ifstatement instead of nestedifstatements(SIM102)
255-257: Simplify nestedifstatements inhasDuplicateLegsAgain, nested
ifstatements can be simplified to a single condition.Apply this diff:
- if self.strategy.parameter("allowMultipleEntriesPerExpiry", False): - if position.expiryStr != order_expiry: + if self.strategy.parameter("allowMultipleEntriesPerExpiry", False) and position.expiryStr != order_expiry: continue🧰 Tools
🪛 Ruff (0.8.0)
255-256: Use a single
ifstatement instead of nestedifstatements(SIM102)
Tests/specs/order/base_spec.py (2)
159-200: Enhance tests for position limitsWhile you have added tests for position limits, consider adding assertions to verify that positions are not created when limits are exceeded, and that they are created when within limits. This will ensure that the position limit checks are functioning as intended.
Example:
# After setting up conditions where limits are exceeded expect(self.base.buildOrderPosition(self.order)).to(equal([None, None])) # When within limits expect(self.base.buildOrderPosition(self.order)).not_to(equal([None, None]))
267-268: Test the actual logic ofhasDuplicateLegsMocking
self.base.hasDuplicateLegsbypasses the actual logic you want to test. Instead, consider setting up test cases with real data to verify thathasDuplicateLegscorrectly identifies duplicate legs.Example:
# Set up positions with specific legs self.base.context.openPositions = {"pos1": "order1"} self.base.context.allPositions = { "order1": MagicMock( strategyId="test_strategy", legs=[MagicMock(strike=100, contractSide="Sell")] ) } # Call hasDuplicateLegs with an order that should be identified as duplicate expect(self.base.hasDuplicateLegs(self.mock_order)).to(be_true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.python-version(1 hunks)Alpha/Base.py(4 hunks)Alpha/FutureSpread.py(1 hunks)Alpha/Utils/Scanner.py(0 hunks)Alpha/Utils/__init__.py(0 hunks)Initialization/SetupBaseStructure.py(1 hunks)Order/Base.py(2 hunks)Tests/specs/alpha/base_spec.py(3 hunks)Tests/specs/alpha/utils/scanner_spec.py(0 hunks)Tests/specs/order/base_spec.py(2 hunks)Tools/DataHandler.py(3 hunks)
💤 Files with no reviewable changes (3)
- Alpha/Utils/init.py
- Tests/specs/alpha/utils/scanner_spec.py
- Alpha/Utils/Scanner.py
✅ Files skipped from review due to trivial changes (1)
- .python-version
🧰 Additional context used
🪛 Ruff (0.8.0)
Order/Base.py
232-233: Use a single if statement instead of nested if statements
(SIM102)
255-256: Use a single if statement instead of nested if statements
(SIM102)
Tools/DataHandler.py
279-279: SecurityType may be undefined, or defined from star imports
(F405)
279-279: SecurityType may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
281-281: Symbol may be undefined, or defined from star imports
(F405)
Alpha/Base.py
265-265: Insight may be undefined, or defined from star imports
(F405)
Initialization/SetupBaseStructure.py
204-204: SecurityType may be undefined, or defined from star imports
(F405)
205-205: NullOptionAssignmentModel may be undefined, or defined from star imports
(F405)
🔇 Additional comments (17)
Tools/DataHandler.py (4)
68-70: Proper handling of empty symbols list
The added condition correctly checks for an empty symbols list, logging a warning and returning None if necessary.
73-84: Enhanced DTE filtering with detailed logging
The updated code improves the DTE filtering process by adding informative logging statements before and after filtering. This enhances traceability and aids in debugging when no symbols remain after filtering.
243-244: Initialize Greeks after adding option contracts
Initializing the Greeks immediately after adding the option contracts ensures that the necessary calculations are performed for each contract.
290-296: Handle potential exceptions when initializing Greeks
While initializing Greeks, ensure that any exceptions are logged with sufficient detail to aid in debugging. The current exception handling is appropriate.
Tests/specs/alpha/base_spec.py (5)
49-52: Addition of mock dataHandler in test setup
Mocking the dataHandler with getOptionContracts returning None is a good practice to isolate the tests from external dependencies.
101-143: Refactored test setup for update method
Consolidating the common attributes in the before.each block improves test readability and maintainability.
457-458: Correct assertion of updateCharts method call
The use of assert_called_once_with ensures that the updateCharts method is called with the correct symbol.
460-472: Comprehensive tests for isMarketClosed method
The added tests effectively cover scenarios where the market is closed due to warming up or actual market closure, as well as when the market is open.
473-497: Robust tests for check_market_schedule method
The tests encompass various edge cases, ensuring that trades are only allowed within the specified schedule window and respecting the minimum trade distance between trades.
Alpha/Base.py (4)
6-6: Removed unused Scanner import
The removal of Scanner from the imports reflects the codebase's shift away from using it, reducing unnecessary dependencies.
204-205: Initialize last_trade_time in constructor
Adding self.last_trade_time = None properly initializes the attribute used for tracking the timing of trades.
245-263: Refactored update method for clarity and efficiency
The update method now uses isMarketClosed and check_market_schedule methods to determine if it should proceed, improving readability and maintainability.
267-296: Well-implemented market condition checks
The new methods isMarketClosed and check_market_schedule encapsulate market condition logic effectively, aiding in code clarity.
Alpha/FutureSpread.py (1)
Line range hint 67-68: Verify market check functionality after Scanner removal
With the Scanner class being removed, ensure that market checks are properly handled through the Base class inheritance. The current implementation relies on Base for market status checks.
Let's verify the market check implementations:
Also applies to: 71-72
✅ Verification successful
Market check functionality is properly handled through Base class inheritance
The verification confirms that:
FutureSpreadclass inherits fromBaseclass (Alpha/Base.py)Baseclass implements both required market check methods:isMarketClosed()check_market_schedule()
- These methods are actively used and tested as shown by the test files
- The market check functionality is properly maintained after Scanner removal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Base class market check implementations
ast-grep --pattern 'def isMarketClosed'
ast-grep --pattern 'def check_market_schedule'
Length of output: 90
Script:
#!/bin/bash
# Let's try a broader search for market-related methods in Base and the current file
rg -l "class Base"
rg "def.*market" -i
rg "isMarketClosed|check_market_schedule" -i
# Also check for Base class inheritance
rg "class.*\(.*Base.*\)"
Length of output: 3908
🧰 Tools
🪛 Ruff (0.8.0)
50-50: time may be undefined, or defined from star imports
(F405)
Tests/specs/order/base_spec.py (3)
55-66: Mock strategy parameters correctly in tests
The strategy mock is properly set up to return specific parameters using side_effect. This allows for flexible testing of different scenarios based on strategy configurations.
118-125: Test order quantity validation effectively
Modifying orderQuantity and maxOrderQuantity to test the validation logic in buildOrderPosition is appropriate. The assertions correctly check that no position or working order is created when the quantity exceeds the maximum.
221-230: Ensure all strategy parameters are set for position limit tests
In the position limit checks context, make sure that all parameters used in check_position_limits are provided in the side_effect. Missing parameters could lead to unexpected defaults being used in the method.
Tools/DataHandler.py
Outdated
| security = self.context.Securities[symbol.ID.Symbol] | ||
| if security.IsTradable: | ||
| tradable_symbols.append(symbol) | ||
| else: | ||
| non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol}: IsTradable={security.IsTradable}") | ||
|
|
There was a problem hiding this comment.
Prevent potential KeyError when accessing Securities dictionary
Accessing self.context.Securities[symbol.ID.Symbol] may raise a KeyError if the symbol is not present in the Securities dictionary. To ensure robustness, check if the security exists before accessing it.
Apply this diff to handle missing securities:
+ if symbol.ID.Symbol in self.context.Securities:
+ security = self.context.Securities[symbol.ID.Symbol]
+ if security.IsTradable:
+ tradable_symbols.append(symbol)
+ else:
+ non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol}: IsTradable={security.IsTradable}")
+ else:
+ non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol} not found in Securities dictionary")
- security = self.context.Securities[symbol.ID.Symbol]
- if security.IsTradable:
- tradable_symbols.append(symbol)
- else:
- non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol}: IsTradable={security.IsTradable}")📝 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.
| security = self.context.Securities[symbol.ID.Symbol] | |
| if security.IsTradable: | |
| tradable_symbols.append(symbol) | |
| else: | |
| non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol}: IsTradable={security.IsTradable}") | |
| if symbol.ID.Symbol in self.context.Securities: | |
| security = self.context.Securities[symbol.ID.Symbol] | |
| if security.IsTradable: | |
| tradable_symbols.append(symbol) | |
| else: | |
| non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol}: IsTradable={security.IsTradable}") | |
| else: | |
| non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol} not found in Securities dictionary") | |
| if not filteredSymbols: | ||
| self.context.logger.warning("No tradable symbols left after filtering") | ||
| return None | ||
| self.context.logger.warning(f"Tradability filter: {before_tradable} -> {len(filteredSymbols)} symbols") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Corrected logging variable
Ensure that the variable used in the logging statement reflects the current filtered symbols list.
Apply this diff to log the correct number of symbols:
- self.context.logger.warning(f"Tradability filter: {before_tradable} -> {len(filteredSymbols)} symbols")
+ self.context.logger.warning(f"Tradability filter: {before_tradable} -> {len(tradable_symbols)} symbols")Committable suggestion skipped: line range outside the PR's diff.
| # Check max open orders | ||
| max_open = self.strategy.parameter("maxOpenPositions", 2) | ||
| open_orders = len([o for o in self.context.Transactions.GetOpenOrders()]) | ||
| if open_orders >= max_open: |
There was a problem hiding this comment.
Filter open orders by strategy
Similarly, when counting open_orders, ensure that only open orders for the current strategy are included. This prevents interference between different strategies.
Apply this diff to filter open orders:
open_orders = len([
- o for o in self.context.Transactions.GetOpenOrders()
+ o for o in self.context.Transactions.GetOpenOrders()
+ if o.Symbol in self.context.Securities
+ and self.context.Securities[o.Symbol].strategyTag == self.nameTag
])📝 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.
| # Check max open orders | |
| max_open = self.strategy.parameter("maxOpenPositions", 2) | |
| open_orders = len([o for o in self.context.Transactions.GetOpenOrders()]) | |
| if open_orders >= max_open: | |
| # Check max open orders | |
| max_open = self.strategy.parameter("maxOpenPositions", 2) | |
| open_orders = len([ | |
| o for o in self.context.Transactions.GetOpenOrders() | |
| if o.Symbol in self.context.Securities | |
| and self.context.Securities[o.Symbol].strategyTag == self.nameTag | |
| ]) | |
| if open_orders >= max_open: |
| return Base.orderCount | ||
|
|
There was a problem hiding this comment.
Fix usage of self in getNextOrderId
The getNextOrderId method is currently decorated with @staticmethod but references self.context, which will raise an error because self is not available in static methods. You should remove the @staticmethod decorator to make it an instance method.
Apply this diff to correct the method:
- @staticmethod
def getNextOrderId(self):
try:
max_order_id = max(orderId for _, orderId in self.context.openPositions.items())
except:
max_order_id = 0
if max_order_id > 0 and Base.orderCount == 0:
Base.orderCount = max_order_id + 1
else:
Base.orderCount += 1
return Base.orderCount📝 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.
| return Base.orderCount | |
| def getNextOrderId(self): | |
| try: | |
| max_order_id = max(orderId for _, orderId in self.context.openPositions.items()) | |
| except: | |
| max_order_id = 0 | |
| if max_order_id > 0 and Base.orderCount == 0: | |
| Base.orderCount = max_order_id + 1 | |
| else: | |
| Base.orderCount += 1 | |
| return Base.orderCount |
| def hasReachedMaxActivePositions(self) -> bool: | ||
| """Check if maximum number of active positions has been reached.""" | ||
| openPositionsByStrategy = { | ||
| tag: pos for tag, pos in self.context.openPositions.items() | ||
| if self.context.allPositions[pos].strategyTag == self.nameTag | ||
| } | ||
| workingOrdersByStrategy = { | ||
| tag: order for tag, order in self.context.workingOrders.items() | ||
| if order.strategyTag == self.nameTag | ||
| } | ||
|
|
||
| return (len(openPositionsByStrategy) + len(workingOrdersByStrategy)) >= self.maxActivePositions | ||
|
|
There was a problem hiding this comment.
Initialize maxActivePositions and maxOpenPositions attributes
The methods hasReachedMaxActivePositions and hasReachedMaxOpenPositions reference self.maxActivePositions and self.maxOpenPositions, but these attributes are not initialized in the __init__ method. This will result in an AttributeError.
Apply this diff to initialize the attributes:
def __init__(self, context, strategy):
self.context = context
# Set the logger
self.logger = Logger(context, className=type(self).__name__, logLevel=context.logLevel)
self.strategy = strategy
# Set default name (use the class name)
self.name = strategy.name
# Set the Strategy Name (optional)
self.nameTag = strategy.nameTag
# Initialize the contract utils
self.contractUtils = ContractUtils(context)
+ # Initialize position limits
+ self.maxActivePositions = self.strategy.parameter("maxActivePositions", 1)
+ self.maxOpenPositions = self.strategy.parameter("maxOpenPositions", 1)📝 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 hasReachedMaxActivePositions(self) -> bool: | |
| """Check if maximum number of active positions has been reached.""" | |
| openPositionsByStrategy = { | |
| tag: pos for tag, pos in self.context.openPositions.items() | |
| if self.context.allPositions[pos].strategyTag == self.nameTag | |
| } | |
| workingOrdersByStrategy = { | |
| tag: order for tag, order in self.context.workingOrders.items() | |
| if order.strategyTag == self.nameTag | |
| } | |
| return (len(openPositionsByStrategy) + len(workingOrdersByStrategy)) >= self.maxActivePositions | |
| def __init__(self, context, strategy): | |
| self.context = context | |
| # Set the logger | |
| self.logger = Logger(context, className=type(self).__name__, logLevel=context.logLevel) | |
| self.strategy = strategy | |
| # Set default name (use the class name) | |
| self.name = strategy.name | |
| # Set the Strategy Name (optional) | |
| self.nameTag = strategy.nameTag | |
| # Initialize the contract utils | |
| self.contractUtils = ContractUtils(context) | |
| # Initialize position limits | |
| self.maxActivePositions = self.strategy.parameter("maxActivePositions", 1) | |
| self.maxOpenPositions = self.strategy.parameter("maxOpenPositions", 1) |
| active_positions = len([p for p in self.context.Portfolio.Values if p.Invested]) | ||
| if active_positions >= max_active: | ||
| return False |
There was a problem hiding this comment.
Filter active positions by strategy
In check_position_limits, active_positions counts all invested positions in the portfolio, including those not related to the current strategy. This can lead to incorrect limit checks if multiple strategies are used. Filter the positions to include only those belonging to the current strategy.
Apply this diff to filter positions:
active_positions = len([
- p for p in self.context.Portfolio.Values if p.Invested
+ p for p in self.context.Portfolio.Values
+ if p.Invested and p.Symbol in self.context.openPositions.values()
+ and self.context.allPositions[self.context.openPositions[p.Symbol]].strategyTag == self.nameTag
])Committable suggestion skipped: line range outside the PR's diff.
|
|
There was a problem hiding this comment.
Adjust test calls to getNextOrderId
Since getNextOrderId is no longer a @staticmethod, you should call it on the instance (self.base) rather than the class (Base).
Apply this diff to update the test:
- first_id = Base.getNextOrderId()
- second_id = Base.getNextOrderId()
+ first_id = self.base.getNextOrderId()
+ second_id = self.base.getNextOrderId()Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Order/Base.py (2)
232-233: Simplify nestedifstatementsYou can combine the nested
ifstatements into a singleifstatement for better readability.Apply this diff:
- if self.strategy.parameter("checkForDuplicatePositions", True): - if self.hasDuplicateLegs(order): + if self.strategy.parameter("checkForDuplicatePositions", True) and self.hasDuplicateLegs(order): return False🧰 Tools
🪛 Ruff (0.8.0)
232-233: Use a single
ifstatement instead of nestedifstatements(SIM102)
255-256: Simplify nestedifstatementsYou can combine the nested
ifstatements into a singleifstatement for better readability.Apply this diff:
- if self.strategy.parameter("allowMultipleEntriesPerExpiry", False): - if position.expiryStr != order_expiry: + if self.strategy.parameter("allowMultipleEntriesPerExpiry", False) and position.expiryStr != order_expiry: continue🧰 Tools
🪛 Ruff (0.8.0)
255-256: Use a single
ifstatement instead of nestedifstatements(SIM102)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
.python-version(1 hunks)Alpha/Base.py(4 hunks)Alpha/FutureSpread.py(1 hunks)Alpha/Utils/Scanner.py(0 hunks)Alpha/Utils/__init__.py(0 hunks)Initialization/SetupBaseStructure.py(1 hunks)Order/Base.py(2 hunks)Tests/specs/alpha/base_spec.py(3 hunks)Tests/specs/alpha/utils/scanner_spec.py(0 hunks)Tests/specs/order/base_spec.py(2 hunks)Tools/DataHandler.py(3 hunks)
💤 Files with no reviewable changes (3)
- Alpha/Utils/init.py
- Tests/specs/alpha/utils/scanner_spec.py
- Alpha/Utils/Scanner.py
🧰 Additional context used
🪛 Ruff (0.8.0)
Tools/DataHandler.py
279-279: SecurityType may be undefined, or defined from star imports
(F405)
279-279: SecurityType may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
281-281: Symbol may be undefined, or defined from star imports
(F405)
Alpha/Base.py
265-265: Insight may be undefined, or defined from star imports
(F405)
Initialization/SetupBaseStructure.py
204-204: SecurityType may be undefined, or defined from star imports
(F405)
205-205: NullOptionAssignmentModel may be undefined, or defined from star imports
(F405)
Order/Base.py
232-233: Use a single if statement instead of nested if statements
(SIM102)
255-256: Use a single if statement instead of nested if statements
(SIM102)
🔇 Additional comments (24)
Order/Base.py (6)
38-40: Position limits are appropriately checked before building the order
The added check ensures that orders respect position limits defined in the strategy.
190-191:
Fix usage of self in getNextOrderId
The getNextOrderId method is still decorated with @staticmethod but references self.context, which will raise an error because self is not available in static methods. You should remove the @staticmethod decorator to make it an instance method.
Apply this diff to correct the method:
- @staticmethod
def getNextOrderId(self):
try:
max_order_id = max(orderId for _, orderId in self.context.openPositions.items())
except:
max_order_id = 0
if max_order_id > 0 and Base.orderCount == 0:
Base.orderCount = max_order_id + 1
else:
Base.orderCount += 1
return Base.orderCount192-204:
Initialize maxActivePositions and maxOpenPositions attributes
The methods hasReachedMaxActivePositions and hasReachedMaxOpenPositions reference self.maxActivePositions and self.maxOpenPositions, but these attributes are not initialized in the __init__ method. This will result in an AttributeError.
Apply this diff to initialize the attributes:
def __init__(self, context, strategy):
self.context = context
# Set the logger
self.logger = Logger(context, className=type(self).__name__, logLevel=context.logLevel)
self.strategy = strategy
# Set default name (use the class name)
self.name = strategy.name
# Set the Strategy Name (optional)
self.nameTag = strategy.nameTag
# Initialize the contract utils
self.contractUtils = ContractUtils(context)
+ # Initialize position limits
+ self.maxActivePositions = self.strategy.parameter("maxActivePositions", 1)
+ self.maxOpenPositions = self.strategy.parameter("maxOpenPositions", 1)221-223:
Filter active positions by strategy
In check_position_limits, active_positions counts all invested positions in the portfolio, including those not related to the current strategy. This can lead to incorrect limit checks if multiple strategies are used. Filter the positions to include only those belonging to the current strategy.
Apply this diff to filter positions:
active_positions = len([
- p for p in self.context.Portfolio.Values if p.Invested
+ p for p in self.context.Portfolio.Values
+ if p.Invested and p.Symbol in self.context.openPositions.values()
+ and self.context.allPositions[self.context.openPositions[p.Symbol]].strategyTag == self.nameTag
])227-228:
Filter open orders by strategy
Ensure that only open orders for the current strategy are included when counting open_orders. This prevents interference between different strategies.
Apply this diff to filter open orders:
open_orders = len([
- o for o in self.context.Transactions.GetOpenOrders()
+ o for o in self.context.Transactions.GetOpenOrders()
+ if o.Symbol in self.context.Securities
+ and self.context.Securities[o.Symbol].strategyTag == self.nameTag
])260-265:
Verify comparison logic in hasDuplicateLegs
The comparison in the nested loops may not correctly identify duplicate legs due to potential mismatches in data types or values for leg.contractSide and order["contractSide"][contract.Symbol]. Ensure that the contract sides are consistent and properly comparable.
Review and, if necessary, adjust the comparison to:
- Use consistent data types (e.g., both as strings or both as integers).
- Confirm that
contractSidevalues match the expected format.
Tests/specs/order/base_spec.py (3)
55-66: Strategy parameters are correctly mocked for tests
The setup of self.base.strategy.parameter provides appropriate test parameters.
118-124: Tests for validating order quantity are correctly implemented
The test cases appropriately check that orders exceeding maxOrderQuantity are rejected.
157-158:
Adjust test calls to getNextOrderId
Since getNextOrderId is no longer a @staticmethod, you should call it on the instance (self.base) rather than the class (Base).
Apply this diff to update the test:
- next_id = Base.getNextOrderId()
+ next_id = self.base.getNextOrderId()Initialization/SetupBaseStructure.py (1)
204-205:
Import SecurityType and NullOptionAssignmentModel explicitly
The use of SecurityType and NullOptionAssignmentModel may lead to undefined name warnings due to star imports. To address this, import these classes explicitly.
Apply this diff to import the missing names:
+from QuantConnect import SecurityType, NullOptionAssignmentModel🧰 Tools
🪛 Ruff (0.8.0)
204-204: SecurityType may be undefined, or defined from star imports
(F405)
205-205: NullOptionAssignmentModel may be undefined, or defined from star imports
(F405)
Tests/specs/alpha/base_spec.py (4)
49-52: Mock dataHandler is correctly added for testing
The addition of self.mock_data_handler simulates the dataHandler for testing purposes.
101-143: Enhanced test setup for the update method
The test setup includes all necessary mocks and initializations to accurately test the update method under various conditions.
459-472: Tests for market status detection are well-implemented
The test cases accurately check the isMarketClosed method under different scenarios, ensuring correct behavior during warm-up and market hours.
473-497: Market schedule checks are thoroughly tested
The check_market_schedule method is tested for various conditions, including trading windows and minimum trade intervals, ensuring adherence to the trading schedule.
Alpha/Base.py (5)
204-204: Initialization of last_trade_time is appropriate
The addition of self.last_trade_time = None ensures that the tracking of the last trade time is properly initialized.
245-248: Performance tracking update is correct
Including performance tracking with self.context.performance.OnUpdate(data) enhances the monitoring of algorithm performance.
267-270: Undefined self.underlyingSymbol in isMarketClosed method
As mentioned earlier, self.underlyingSymbol is not initialized in the class. Ensure it is defined to prevent errors.
263-263: Import Insight explicitly to resolve undefined name
The name Insight may be undefined due to star imports. To ensure Insight.Group(insights) works correctly, import Insight explicitly.
Apply this diff to import Insight:
+from QuantConnect.Algorithm.Framework import Insight272-296: Verify that schedule parameters are correctly initialized
In the check_market_schedule method, ensure that self.scheduleStartTime, self.scheduleStopTime, and self.minimumTradeScheduleDistance are properly initialized and accessible as instance attributes.
Run the following script to confirm that schedule parameters are set:
✅ Verification successful
Schedule parameters are properly initialized through class defaults
The verification shows that the schedule parameters are correctly defined in the DEFAULT_PARAMETERS dictionary of the Base class:
scheduleStartTimeis initialized totime(9, 30, 0)scheduleStopTimeis set toNoneby defaultminimumTradeScheduleDistanceis set totimedelta(days=1)
These parameters are accessible through class parameter merging (DEFAULT_PARAMETERS + PARAMETERS) and are properly used in the check_market_schedule method.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if schedule parameters are defined in the Base class.
# Search for assignments of schedule parameters in the class definition
rg -A 5 -B 5 'class Base' Alpha/Base.py | rg 'self\.schedule(StartTime|StopTime|Frequency|MinimumTradeScheduleDistance)'
Length of output: 122
Script:
#!/bin/bash
# Let's search for the initialization of these parameters in the Base class
# First, let's check the constructor and any initialization methods
ast-grep --pattern 'class Base {
$$$
def __init__($$$) {
$$$
}
$$$
}'
# Also search for any property definitions or assignments of these parameters
rg "scheduleStartTime|scheduleStopTime|minimumTradeScheduleDistance" Alpha/Base.py
Length of output: 472
Script:
#!/bin/bash
# Let's get more context around where these parameters are defined
rg -B 5 -A 5 '"scheduleStartTime": time\(' Alpha/Base.py
# Also check if these are being set from configuration or parameters
rg -B 2 -A 2 "def __init__" Alpha/Base.py
Length of output: 843
Script:
#!/bin/bash
# Let's check how these default parameters are used in the class
rg -B 3 -A 3 "DEFAULT_PARAMETERS" Alpha/Base.py
# Also check for any parameter initialization or configuration loading
ast-grep --pattern 'def initialize($$$) {
$$$
}'
Length of output: 880
.python-version (1)
1-1: Python version update is acceptable
Updating the Python version to 3.8.20 keeps the environment up-to-date with the latest patches.
Alpha/FutureSpread.py (1)
52-53: Wing size increase requires careful consideration
The increase of putWingSize and callWingSize from 20 to 50 significantly widens the spreads. This change could impact:
- Risk/reward profile
- Margin requirements
- Premium collection
- Position sizing
Please review and justify this aggressive parameter change, or consider aligning with the more conservative wing sizes (5-20 points) used elsewhere in the codebase.
Tools/DataHandler.py (3)
68-70: Logging enhancement improves observability
Adding a warning when no initial symbols are provided enhances the visibility of the filtering process.
243-244: Initialize Greeks after adding contracts
Calling _initializeGreeks ensures that option Greeks are calculated promptly after adding the contracts, which is essential for accurate option pricing and strategy implementation.
276-297: Import SecurityType, OptionRight, and Symbol explicitly
The use of SecurityType, OptionRight, and Symbol in _initializeGreeks may cause NameError if they are not imported explicitly.
Apply this diff to import the missing names:
+from QuantConnect import SecurityType, OptionRight, Symbol🧰 Tools
🪛 Ruff (0.8.0)
279-279: SecurityType may be undefined, or defined from star imports
(F405)
279-279: SecurityType may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
280-280: OptionRight may be undefined, or defined from star imports
(F405)
281-281: Symbol may be undefined, or defined from star imports
(F405)
| if self.isMarketClosed(): | ||
| return [] | ||
|
|
There was a problem hiding this comment.
Potential issue: self.underlyingSymbol may not be defined
The method isMarketClosed uses self.underlyingSymbol, but self.underlyingSymbol is not defined in the Base class. This may lead to an AttributeError at runtime.
Apply this diff to define self.underlyingSymbol in the __init__ method:
def __init__(self, context):
self.context = context
self.name = type(self).__name__ # Set default name (use the class name)
self.nameTag = self.name # Set the Strategy Name (optional)
+ self.underlyingSymbol = context.symbol # Define underlyingSymbol
self.logger = Logger(context, className=type(self).__name__, logLevel=context.logLevel) # Set the loggerCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Tools/DataHandler.py (1)
133-156:⚠️ Potential issueAdd safety check when accessing Securities dictionary.
The code could raise a KeyError when accessing
self.context.Securities[symbol.ID.Symbol]. Add a safety check:- for symbol in filteredSymbols[:5]: # Sample first 5 symbols for detailed logging - security = self.context.Securities[symbol.ID.Symbol] - if security.IsTradable: - tradable_symbols.append(symbol) - else: - non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol}: IsTradable={security.IsTradable}") + for symbol in filteredSymbols[:5]: # Sample first 5 symbols for detailed logging + if symbol.ID.Symbol in self.context.Securities: + security = self.context.Securities[symbol.ID.Symbol] + if security.IsTradable: + tradable_symbols.append(symbol) + else: + non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol}: IsTradable={security.IsTradable}") + else: + non_tradable_reasons.append(f"Symbol {symbol.ID.Symbol} not found in Securities dictionary")
🧹 Nitpick comments (1)
Tools/DataHandler.py (1)
98-132: Consider extracting DTE selection logic into helper methods.The DTE selection logic is complex and handles multiple scenarios. Consider extracting into helper methods for better maintainability:
_selectExpiryDate_handleDynamicDTESelection_filterMultipleEntriesdef optionChainProviderFilter(self, symbols, min_strike_rank, max_strike_rank, minDte, maxDte): # ... existing code ... - # Get unique expiry dates - expiry_dates = sorted(set(symbol.ID.Date for symbol in filteredSymbols), reverse=True) - - # Handle dynamic DTE selection if enabled - if (hasattr(self.strategy, 'dynamicDTESelection') and self.strategy.dynamicDTESelection and - hasattr(self.context, 'recentlyClosedDTE') and self.context.recentlyClosedDTE): - valid_closed_trades = [ - trade for trade in self.context.recentlyClosedDTE - if trade["closeDte"] >= minDte - ] - if valid_closed_trades: - last_closed_dte = valid_closed_trades[0]["closeDte"] - # Find expiry date closest to last closed DTE - target_expiry = min(expiry_dates, - key=lambda x: abs((x.date() - self.context.Time.date()).days - last_closed_dte)) - filteredSymbols = [s for s in filteredSymbols if s.ID.Date == target_expiry] - else: - # Use furthest/earliest expiry based on useFurthestExpiry - selected_expiry = expiry_dates[0 if self.strategy.useFurthestExpiry else -1] - filteredSymbols = [s for s in filteredSymbols if s.ID.Date == selected_expiry] + filteredSymbols = self._selectExpiryDate(filteredSymbols, minDte) + + if not self._allowMultipleEntries(): + filteredSymbols = self._filterMultipleEntries(filteredSymbols)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Tests/specs/alpha/base_spec.py(7 hunks)Tests/specs/tools/data_handler_spec.py(3 hunks)Tools/DataHandler.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
Tools/DataHandler.py
76-76: timedelta may be undefined, or defined from star imports
(F405)
303-303: SecurityType may be undefined, or defined from star imports
(F405)
303-303: SecurityType may be undefined, or defined from star imports
(F405)
304-304: OptionRight may be undefined, or defined from star imports
(F405)
304-304: OptionRight may be undefined, or defined from star imports
(F405)
304-304: OptionRight may be undefined, or defined from star imports
(F405)
305-305: Symbol may be undefined, or defined from star imports
(F405)
Tests/specs/alpha/base_spec.py
16-16: Tools.DataHandler imported but unused
Remove unused import: Tools.DataHandler
(F401)
18-18: Tests.mocks.algorithm_imports.SecurityType imported but unused
Remove unused import
(F401)
18-18: Tests.mocks.algorithm_imports.Resolution imported but unused
Remove unused import
(F401)
18-18: Tests.mocks.algorithm_imports.OptionRight imported but unused
Remove unused import
(F401)
19-19: Tests.mocks.algorithm_imports.TradeBar imported but unused
Remove unused import
(F401)
19-19: Tests.mocks.algorithm_imports.PortfolioTarget imported but unused
Remove unused import
(F401)
20-20: Tests.mocks.algorithm_imports.List imported but unused
Remove unused import
(F401)
20-20: Tests.mocks.algorithm_imports.SecurityChanges imported but unused
Remove unused import
(F401)
20-20: Tests.mocks.algorithm_imports.AlphaModel imported but unused
Remove unused import
(F401)
21-21: Tests.mocks.algorithm_imports.Slice imported but unused
Remove unused import
(F401)
🔇 Additional comments (8)
Tests/specs/tools/data_handler_spec.py (2)
Line range hint 118-155: LGTM! Test data setup is comprehensive.
The test data setup effectively covers different strike prices around ATM (95, 100, 105) and properly configures mock objects with all required attributes.
161-164: LGTM! Test assertions are accurate.
The assertions correctly verify that all three strikes (95, 100, 105) are included in the filtered results and that performance is tracked via execution timer.
Tools/DataHandler.py (3)
70-87: LGTM! Enhanced logging improves debuggability.
The added logging provides comprehensive information about the initial state and detailed data for the first 5 symbols, which will help with debugging and monitoring.
🧰 Tools
🪛 Ruff (0.8.2)
76-76: timedelta may be undefined, or defined from star imports
(F405)
176-195: LGTM! Strike filtering logic is robust and well-documented.
The strike filtering implementation is thorough with:
- Clear ATM strike calculation
- Proper bounds checking for strike indices
- Comprehensive logging of strike ranges
300-321: LGTM! Greeks initialization is comprehensive.
The _initializeGreeks method effectively:
- Handles both Option and IndexOption types
- Creates mirror symbols correctly
- Initializes all Greeks (iv, delta, gamma, vega, rho, theta)
- Includes proper error handling
🧰 Tools
🪛 Ruff (0.8.2)
303-303: SecurityType may be undefined, or defined from star imports
(F405)
303-303: SecurityType may be undefined, or defined from star imports
(F405)
304-304: OptionRight may be undefined, or defined from star imports
(F405)
304-304: OptionRight may be undefined, or defined from star imports
(F405)
304-304: OptionRight may be undefined, or defined from star imports
(F405)
305-305: Symbol may be undefined, or defined from star imports
(F405)
Tests/specs/alpha/base_spec.py (3)
24-56: LGTM! MockUnderlying implementation is robust.
The MockUnderlying class is well-designed with:
- Proper singleton pattern implementation
- Clean price management
- Useful reset functionality for test isolation
461-474: LGTM! Market checks tests are comprehensive.
The tests effectively cover all key market scenarios:
- Market closed during warmup
- Closed market detection
- Open market detection
475-499: LGTM! Market schedule tests are thorough.
The tests effectively verify:
- Trading within schedule window
- Prevention of trading outside schedule
- Minimum trade distance requirements
Deprecate the Alpha/Utils/Scanner.py class as we can better implement the same functionality in the other classes.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
Scannerclass and associated tests.