Support exclude_weekends for PeriodicEvent

[guyfreund]

Hi @myrmarachne, opened a small PR to allow excluding weekends for PeriodicEvent object.

For example in demo_scripts/strategies/intraday_strategy.py just use CalculateAndPlaceOrdersPeriodicEvent.exclude_weekends() in line 99.

[myrmarachne]

Hi @guyfreund , thank you for opening the PR. I see that the pipeline doesn't work and I was wondering - did you set the 'QUANTFIN_SECRET' environment variable in your repository?

[guyfreund]

@myrmarachne I didn't set it. To what should I set it?

[guyfreund]

@myrmarachne in addition I believe this addition needs a corresponding unit-test. Can you please guide me on where to locate it?

[myrmarachne]

@myrmarachne I didn't set it. To what should I set it?

The error points to issue in json parsing and not to lacking secret, so it means that somehow this variable "is being found". Maybe it is an issue of the github actions environment - try in that case to simply add QUANTFIN_SECRET secret with value equal to {} (empty dictionary) in your repo.

[myrmarachne]

@myrmarachne in addition I believe this addition needs a corresponding unit-test. Can you please guide me on where to locate it?

You can find the unit tests for events (with e.g. testing the exclude_weekends function for market open or basic tests for the periodic events) in the qf-lib/qf_lib/tests/unit_tests/backtesting/events/time_event/test_rules.py

I would suggest to put the tests either to this file or create a new test file in that directory.

[guyfreund]

@myrmarachne in addition I believe this addition needs a corresponding unit-test. Can you please guide me on where to locate it?

You can find the unit tests for events (with e.g. testing the exclude_weekends function for market open or basic tests for the periodic events) in the qf-lib/qf_lib/tests/unit_tests/backtesting/events/time_event/test_rules.py

I would suggest to put the tests either to this file or create a new test file in that directory.

@myrmarachne Done.

The only thing I'm not sure about is that you cannot initiate an instance of a class that inhertis from PeriodicEvent and after initialization set the instance.exclude_weekends(). You must do that in the level of the class that inherits PeriodicEvent e.g. Periodic15MinutesEvent.exclude_weekends() before calling the constructor of Periodic15MinutesEvent - all of this because of the _events_list attribute of the PeriodicEvent class.

I saw the tests for MarketOpenEvent and MarketCloseEvent init an instance first and then set the instance.exclude_weekends().

Not sure if it is a problem with the current architecture of the events flow or not, but thought it is worth to mention that. WDYT?

[myrmarachne]

@myrmarachne in addition I believe this addition needs a corresponding unit-test. Can you please guide me on where to locate it?

You can find the unit tests for events (with e.g. testing the exclude_weekends function for market open or basic tests for the periodic events) in the qf-lib/qf_lib/tests/unit_tests/backtesting/events/time_event/test_rules.py I would suggest to put the tests either to this file or create a new test file in that directory.

@myrmarachne Done.

The only thing I'm not sure about is that you cannot initiate an instance of a class that inhertis from PeriodicEvent and after initialization set the instance.exclude_weekends(). You must do that in the level of the class that inherits PeriodicEvent e.g. Periodic15MinutesEvent.exclude_weekends() before calling the constructor of Periodic15MinutesEvent - all of this because of the _events_list attribute of the PeriodicEvent class.

I saw the tests for MarketOpenEvent and MarketCloseEvent init an instance first and then set the instance.exclude_weekends().

Not sure if it is a problem with the current architecture of the events flow or not, but thought it is worth to mention that. WDYT?

We are very well aware of this, we thought at some point about changing the way the events are created, however at that point it was complicating the whole architecture and also making it even harder for the user (we didn't want the user to be responsible for instantiating and passing the event objects). Currently in most of the cases user just needs to set the frequency and trading hours on the CalculateAndPlaceOrdersRegularEvent or CalculateAndPlaceOrdersPeriodicEvent. Creation of e.g. Periodic15MinutesEvent would be necessary only in case if the user wants to do some custom strategies, which would follow more events than just the CalculateAndPlaceOrders ... Events.

Regarding your Pull request - I can still see the JSON decoding errors in the pipeline. In case if you already put the empty dictionary as the secret of your project, it means that the issue lies somewhere else. We will try to reproduce this issue on our end and let you know in case if the fix would be on your side.

[guyfreund]

I did set the env var. I added another commit, if you can enable the build we can see if it further reproduces. Thanks @myrmarachne

[mborraty]

Hi @guyfreund!

Thanks a lot for the PR. I've just investigated the issue and it looks like github bug to me. Github uses your QUANTFIN_SECRET instead of the one in original qf-lib repo.

It is easy to fix, could you please do the following:

In the forked repository go to General -> Security -> Secrets -> Actions Click "New repository secret"

Add the secret variable with the following settings: Name: QUANTFIN_SECRET Secret: {}

rebase your repo and push.

Thanks, Kind regards, M.

[guyfreund]

@mborraty done. Can you please approve the build?