Fix/backtest datetime normalization #888
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Timezone comparison using object identity ▹ view | |
|
Redundant datetime normalization calls ▹ view | |
 |
Static method called with instance/class reference ▹ view | |
 |
Incomplete function docstring ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| lumibot/strategies/_strategy.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
lumibot/strategies/_strategy.py
Outdated
| tzinfo = getattr(aware, "tzinfo", None) | ||
| if tzinfo is not None and tzinfo != LUMIBOT_DEFAULT_PYTZ: | ||
| return aware.astimezone(LUMIBOT_DEFAULT_PYTZ) |
There was a problem hiding this comment.
Timezone comparison using object identity 
Tell me more
What is the issue?
The timezone comparison uses object identity (!=) instead of timezone equality, which may fail when comparing equivalent timezone objects that are different instances.
Why this matters
This could cause unnecessary timezone conversions or missed conversions when timezone objects represent the same timezone but are different instances, leading to incorrect datetime handling in backtests.
Suggested change ∙ Feature Preview
Use timezone equality comparison or zone name comparison instead of object identity:
if tzinfo is not None and str(tzinfo) != str(LUMIBOT_DEFAULT_PYTZ):
return aware.astimezone(LUMIBOT_DEFAULT_PYTZ)Or use timezone zone comparison if available.
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| backtesting_start = self._normalize_backtest_datetime(backtesting_start) | ||
| backtesting_end = self._normalize_backtest_datetime(backtesting_end) |
There was a problem hiding this comment.
Redundant datetime normalization calls
Tell me more
What is the issue?
The datetime normalization is called twice for the same values - once in run_backtest and again in verify_backtest_inputs, causing redundant timezone conversion operations.
Why this matters
This creates unnecessary computational overhead by performing the same timezone conversion operations multiple times, which could impact performance especially when running many backtests.
Suggested change ∙ Feature Preview
Pass the already normalized datetime objects to verify_backtest_inputs or modify verify_backtest_inputs to accept pre-normalized datetimes:
self.verify_backtest_inputs(backtesting_start, backtesting_end, already_normalized=True)And update verify_backtest_inputs to skip normalization when already_normalized=True.
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
lumibot/strategies/_strategy.py
Outdated
| @staticmethod | ||
| def _normalize_backtest_datetime(value): | ||
| """Convert backtest boundary datetimes to the LumiBot default timezone.""" | ||
| if value is None: | ||
| return None | ||
| aware = to_datetime_aware(value) | ||
| tzinfo = getattr(aware, "tzinfo", None) | ||
| if tzinfo is not None and tzinfo != LUMIBOT_DEFAULT_PYTZ: | ||
| return aware.astimezone(LUMIBOT_DEFAULT_PYTZ) | ||
| return aware |
There was a problem hiding this comment.
Static method called with instance/class reference 
Tell me more
What is the issue?
The static method _normalize_backtest_datetime cannot access class or instance attributes, but it's being called as self._normalize_backtest_datetime() and cls._normalize_backtest_datetime() in the code.
Why this matters
Static methods don't have access to self or cls, so calling them with instance or class references can lead to confusion and potential issues if the method ever needs to access class/instance state. The method should either be a classmethod if it needs class access, or the calls should use the class name directly.
Suggested change ∙ Feature Preview
Change the static method to a classmethod since it's being called with cls reference:
@classmethod
def _normalize_backtest_datetime(cls, value):
"""Convert backtest boundary datetimes to the LumiBot default timezone."""
if value is None:
return None
aware = to_datetime_aware(value)
tzinfo = getattr(aware, "tzinfo", None)
if tzinfo is not None and tzinfo != LUMIBOT_DEFAULT_PYTZ:
return aware.astimezone(LUMIBOT_DEFAULT_PYTZ)
return awareProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
lumibot/strategies/_strategy.py
Outdated
| def _normalize_backtest_datetime(value): | ||
| """Convert backtest boundary datetimes to the LumiBot default timezone.""" |
There was a problem hiding this comment.
Incomplete function docstring 
Tell me more
What is the issue?
The docstring only states what the function does but lacks parameter and return value documentation.
Why this matters
Without parameter and return value documentation, users won't know what type of input is expected or what type of output is returned.
Suggested change ∙ Feature Preview
"""Convert backtest boundary datetimes to the LumiBot default timezone.
Parameters
----------
value : datetime or None
The datetime value to normalize
Returns
-------
datetime or None
The normalized datetime in LUMIBOT_DEFAULT_PYTZ timezone"""
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
No description provided.