-
Notifications
You must be signed in to change notification settings - Fork 657
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
Add adaptive_bar_ordering to BacktestVenueConfig #2188
Conversation
1efba17
to
81bfc14
Compare
81bfc14
to
2e358b0
Compare
Hi @faysou , First, thank you again for implementing this feature so quickly! While testing it, I had a thought about the configuration name Would you consider renaming it to
The new name would make the feature's purpose more immediately clear to other users who might benefit from this improvement in backtesting accuracy. What are your thoughts on this? :-) |
Yes no problem, I'll make the change later today |
2e358b0
to
8bd7c60
Compare
8bd7c60
to
4ae173a
Compare
ok thank you. I've attempted to add a test, but it's quite difficult, I've left the current status of the code for the test as comment. |
I was also checking, how to debug this (using some Python breakpoint) and found out, that the only way for me was to check the DEBUG logs. Probably it is hard to test easily using Python, as the ticks from bars are generated outside of Python. As an alternative (for missing test coverage), I will be using this feature extensively - so I will guarantee correct working. |
yes ticks from bars are in cython. you can add log or print statements then recompile with make build-debug |
Yeah, I am really happy I learnt this from you today - how to build / compile this locally. That makes a huge difference in testing /dev and brings new options how to test everything. Very happy for this 👍 |
Yes, I've saved you some time, but you have good ideas as well, I like that. Most people never have any idea 😂 |
4ae173a
to
7104aea
Compare
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.
Noted that the test is WIP and being actively worked on 👌
I was thinking about test - it probably belongs here: but have no idea, how could one technically verify from Python, that Bar was splitted into 4 prices as this is done in Cython code. I can't simply find any link to internal data, that could be checked in the test to verify. Maybe one option would be to verify it by the external effects -> for examle when we fire 2 limit order (1st near top of the bar, 2nd near bottom of the bar) and we can check in which order they are filled. By this approach could be confusing and too far from original simple point we need to test. |
@stefansimik I also think thats a good location for the test, and @faysou is currently working on one. I think testing the external effects of setting the config option The @filipmacek is currently working on the Rust port for I also just pushed this refactoring.
|
Like the new name even more = 100% clear and self-descriptive. 👌👌 |
Pull Request
Add adaptive_bar_ordering to BacktestVenueConfig
Executes Low before High if Low is closer to Open, and vice versa
Type of change
How has this change been tested?
Attempted to add a test in test_matching_engine.py, left it as comment for further work