Refactor time-based bar aggregation #2675
Conversation
cjdsellers
changed the title
cjdsellers
left a comment
cjdsellers
left a comment
There was a problem hiding this comment.
Thanks for all the additional test coverage, this is valuable.
nautilus_trader/data/aggregation.pyx
Outdated
|
|
||
| cdef BarAggregation aggregation = self.bar_type.spec.aggregation | ||
|
|
||
| if type(self._time_bars_origin) is not pd.Timedelta and type(self._time_bars_origin) is not pd.DateOffset: |
There was a problem hiding this comment.
This could also be validated in the constructor, the Condition.type function may be useful for standadization.
nautilus_trader/data/aggregation.pyx
Outdated
| start_time += pd.DateOffset(months=step) | ||
|
|
||
| start_time -= pd.DateOffset(months=step) | ||
|
|
There was a problem hiding this comment.
Lets not add a blank between keywords in an if-elif-else block, I think this makes the code less cohesive and harder to reason about.
|
|
||
| return start_time | ||
|
|
||
|
|
There was a problem hiding this comment.
Methods should be separated by a single blank, functions are separated by two blanks.
This would be autoformatted if it was a .py, except our Python formatter can't parse this Cython.
|
|
||
| # Delay to reset of _batch_next_close_ns to allow the creation of a last histo bar | ||
| # when transitioning to regular bars | ||
| # TODO: Refactor this, it is needless now (the comment above doesn't apply) |
There was a problem hiding this comment.
I can't comment so much on this batch mode, as it's a later addition and I don't have full context.
There was a problem hiding this comment.
I disagree with the useless comment, that's useful when transitioning from the aggregation of a request to the start of a backtest. There are many edge cases, this code was actually a hard edge case to solve.
There was a problem hiding this comment.
Maybe this edge case wouldn't be necessary with fire immediately as you mention. Because what I do here is basically allow to build a bar without a timer, when in a case where the first bar arrives at the same as the beginning of a backtest.
| cdef int step = self.bar_type.spec.step | ||
|
|
||
| if self.bar_type.spec.aggregation != BarAggregation.MONTH: | ||
| # On receiving this event, timer should now have a new `next_time_ns` |
There was a problem hiding this comment.
Is this where we're solving the next_time_ns not being updated along with the clocks internal timer?
There was a problem hiding this comment.
If I remember correctly, then yes.
nautilus_trader/data/aggregation.pyx
Outdated
| # _build_on_next_tick is used to avoid a race condition between a data update and a TimeEvent from the timer | ||
| self._build_on_next_tick = True | ||
| self._stored_close_ns = self.next_close_ns | ||
| self._stored_close_ns = event.ts_event |
There was a problem hiding this comment.
I think this part of the logic is harder to reason about. It could be fixing a bug, especially if self.next_close_ns wasn't being updated -- but if it was, we're changing this from next close to current event time?
This was also the block we were considering removing? The comprehensive test coverage may help us to make a decision here.
� Conflicts: � nautilus_trader/data/aggregation.pyx � tests/unit_tests/data/test_aggregation.py
…per function in actor.pyx (#5)
Enabled CI/CD builds on all pull requests Setup Copilot agent, so he can build and test Fix CI/CD disk space memory leaks during parallel compilation with uv and Rust
|
|
|
Closing this, as the change have been/will be distributed across different PRs. |
4 participants
Draft Pull Request
Summary
interval_typetype from str to enumself.next_close_nsdoesn't change, when no tick is sentRelated Issues/PRs
https://nt-dist.stty.cz/issues/20250509-1-first-bar-not-generated-on-millisecond-bug/
Type of change
Breaking change details (if applicable)
Release notes
RELEASES.mdthat follows the existing conventions (when applicable)Testing
Ensure new or changed logic is covered by tests.