Skip to content
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

refactor: move IO responsibility out of Processor trait #134

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

garyttierney
Copy link

De-duplicates the event reading/data management code contained within each processor and delegates it to the Backtester to handle.

Standalone version of the changes in #124, but without any of the parallel reader code.

@garyttierney
Copy link
Author

Oops, few todo!()s left in there. Will fix those.

@garyttierney garyttierney changed the title Consolidate event reading/processing code refactor: remove IO resonpsibility out of Processor trait Aug 27, 2024
@garyttierney garyttierney changed the title refactor: remove IO resonpsibility out of Processor trait refactor: remove IO responsibility out of Processor trait Aug 27, 2024
@nkaz001
Copy link
Owner

nkaz001 commented Aug 27, 2024

Can you write test cases? I will look into this separately. But since the code is growing and refactored widely, it becomes hard to check if works as expected (the backtesting result is the same as the previous).

@garyttierney
Copy link
Author

Yes, I think we can start by testing that the ProcessorState is correctly doing what process_data() was doing previously. From there I think we can write some more tests now that it's a bit easier to setup the backtester with a given mock state.

@nkaz001
Copy link
Owner

nkaz001 commented Aug 27, 2024

It looks good. It can be merged after the tests have been completed.

@garyttierney
Copy link
Author

@nkaz001 added a simple unit test that runs the actual Backtester and will create a couplee more. What do you think about this testing strategy, and is there anything specific you think we should check?

@garyttierney garyttierney changed the title refactor: remove IO responsibility out of Processor trait refactor: move IO responsibility out of Processor trait Aug 29, 2024
@nkaz001
Copy link
Owner

nkaz001 commented Sep 6, 2024

IMO, the original implementation needs refactoring to make it more unit testable at the function level. Currently, I am testing it by running the tutorials. Similarly, it would be better to run a testing strategy, which generates a high turnover, with test data and verifying results, such as the final P&L and turnover, for a holistic test.

@garyttierney
Copy link
Author

IMO, the original implementation needs refactoring to make it more unit testable at the function level. Currently, I am testing it by running the tutorials.

I agree. I've started to move handling of event intents out of Backtester and into the processor state, but I'm not sure it's the correct way to go. For now it does allow easily checking that the correct flow is followed when end-of-data is reached, etc. I'm exploring a few avenues for breaking it up into easily testable units, but keen to here if you had any ideas in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants