Add TimeInForce.Day support in backtesting and IB brokerage #1938

StefanoRaggi · 2018-04-30T20:18:29Z

Description

This PR includes some common changes to support TimeInForce types:

New ITimeInForceHandler interface and GoodTilCancelled and Day implementations
Base OrderProperties class, instantiated in QCAlgorithm constructor

Both unit tests and regression algorithms have been added.

This feature is currently supported only in backtesting and with Interactive Brokers brokerage.

Related Issue

QuantConnect/Lean.Brokerages.InteractiveBrokers#27

Motivation and Context

Previously we only supported GTC time in force.

Requires Documentation Change

No.

How Has This Been Tested?

New unit tests, new regression test and IB live testing.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Non-functional change (xml comments/documentation/ect)

Checklist:

My code follows the code style of this project.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
My branch follows the naming convention bug-<issue#>-<description or feature-<issue#>-<description>

mchandschuh

Overall I think this is looking like a great foundation for the others to come. A few comments about dependency injection of the market hours database and the day handler impl. Let me know your thoughts :)

mchandschuh · 2018-04-30T20:24:57Z

Algorithm.Python/TimeInForceAlgorithm.py

+### <meta name="tag" content="using data" />
+### <meta name="tag" content="using quantconnect" />
+### <meta name="tag" content="trading and orders" />
+class TimeInForceAlgorithm(QCAlgorithm):


Thanks for adding a python version too!

mchandschuh · 2018-04-30T20:25:55Z

Brokerages/Backtesting/BacktestingBrokerage.cs

@@ -457,5 +480,27 @@ private void SetPendingOrder(Order order)
            // only save off clones!
            _pending[order.Id] = order.Clone();
        }
+
+        private ITimeInForceHandler GetTimeInForceHandler(TimeInForce timeInForce)


instead of having this function, it might be more concise to simply initialize the dictionaries with all the handlers already mapped, and then replace calls to this function with explicit dictionary access, _timeInForceHandlers[timeInForce]

mchandschuh · 2018-04-30T20:29:03Z

Common/Orders/TimeInForces/DayTimeInForceHandler.cs

+        {
+            var exchangeHours = MarketHoursDatabase
+                .FromDataFolder()
+                .GetExchangeHours(order.Symbol.ID.Market, order.Symbol, order.Symbol.SecurityType);


Instead of accessing the FromDataFolder() here, I think it may be better to accept the market hours database instance in the constructor, and then who ever creates this type can provide their own market hours -- we'll pretty much always use it from the data folder, but this can make this class testable w/out having a market-hours-database.json file in the correct place.

mchandschuh · 2018-04-30T20:30:28Z

Common/Orders/TimeInForces/DayTimeInForceHandler.cs

+
+            var time = _algorithm.UtcTime.ConvertFromUtc(exchangeHours.TimeZone);
+
+            return !exchangeHours.IsOpen(time, false);


Is this correct? Isn't it possible to move to the next day and have IsOpen return true again. Is it more correct to compare the order's submit date and the current date (in the security's time zone) ?

What happens with 24 hour markets? If I say time in force = day at 23:59PM - does it cancel at the stroke of midnight?

Good point, this might need to be different per security type:

Forex: both FXCM and Oanda use 5 PM New York time as day close

Crypto: GDAX does not support Day, so maybe we should use midnight UTC here?

mchandschuh · 2018-04-30T20:31:28Z

Tests/Common/Orders/TimeInForces/TimeInForceHandlerTests.cs

+        }
+
+        [Test]
+        public void DayTimeInForceOrderExpiresAtMarketClose()


This assumes a couple things -- one, that we get data at market close (almost guaranteed) or that we get data before the next market open (less of a guarantee) -- see comments in the day handler impl.

mchandschuh

Comments regarding dependencies and the edits to IAlgorithm -- also, spelling of canceled for consistency with OrderStatus.Canceled.

Question: I know this isn't implemented in this PR yet, but I'm starting to wonder how a user would set GoodTilDate -- in an ideal world the user would be able to do:

DefaultOrderProperties.TimeInForce = TimeSpan.FromDays(14);

to set a GTD 2 weeks away. If we want to support syntax like this, then I think we'll need to make TimeInForce a class, and then we can define implicit conversions. For example, converting TimeSpan.Zero to TimeInForce would equate to ImmediateOrCancel and a non-zero value would convert to GTD -- @jaredbroad -- do you have thoughts regarding the API here?

I just don't want users to have to do the following:

DefaultOrderProperties.TimeInForce = TimeInForce.GoodTilDate;
DefaultOrderProperties.TimeInForceTimeSpan = TimeSpan.FromDays(14);

mchandschuh · 2018-05-01T13:12:33Z

Common/Orders/TimeInForces/DayTimeInForceHandler.cs

+            var exchangeHours = _marketHoursDatabase.GetExchangeHours(order.Symbol.ID.Market, order.Symbol, order.Symbol.SecurityType);
+
+            var orderTime = order.Time.ConvertFromUtc(exchangeHours.TimeZone);
+            var time = _algorithm.UtcTime.ConvertFromUtc(exchangeHours.TimeZone);


This algorithm dependency seems heavy handed. It looks like we're only using the algorithm to get the current time. A common pattern in lean models is to accept the Security object in the method. Accepting the security object would remove the algorithm dependency and the market hours database dependency, since the current time and the exchange hours can both be directly resolved form the security object.

mchandschuh · 2018-05-01T13:13:34Z

Algorithm.CSharp/TimeInForceAlgorithm.cs

+                // This order has a default time in force of GoodTilCancelled,
+                // it will never expire and will not be cancelled automatically.
+
+                _gtcOrderTicket = LimitOrder(_symbol, 10, 160m);


For consistency, perhaps set DefaultOrderProperties.TimeInForce = TimeInforce.GoodTilCancelled

mchandschuh · 2018-05-01T13:37:41Z

Brokerages/Backtesting/BacktestingBrokerage.cs

@@ -38,6 +39,7 @@ public class BacktestingBrokerage : Brokerage
        private readonly ConcurrentDictionary<int, Order> _pending;
        private readonly object _needsScanLock = new object();
        private readonly HashSet<Symbol> _pendingOptionAssignments = new HashSet<Symbol>();
+        private readonly Dictionary<TimeInForce, ITimeInForceHandler> _timeInForceHandlers = new Dictionary<TimeInForce, ITimeInForceHandler>();


Hopefully we can change the implementations of ITimeInForceHandler such that they pull all information from the method parameters leading to a parameterless constructor, and then we can inline the creating of this map here of the form:

private readonly Dictionary<TimeInForce, ITimeInForceHandler> _timeInForceHandlers = new Dictionary<TimeInForce, ITimeInForceHandler> { { TimeInForce.GoodTilCanceled, new GoodTilCanceledTimeInForceHandler() } };

mchandschuh · 2018-05-01T13:38:42Z

Brokerages/InteractiveBrokers/InteractiveBrokersBrokerage.cs

+
+                case TimeInForce.GoodTilCancelled:
+                default:
+                    return IB.TimeInForce.GoodTillCancel;


Something I'm just realizing. In english 'canceled' and 'cancelled' are both equally correct spellings. We already use 'canceled' with a single L in OrderStatus.Canceled -- so let's keep the spelling the same for all our sanity :P

mchandschuh · 2018-05-01T13:39:13Z

Common/Brokerages/FxcmBrokerageModel.cs

+            if (order.TimeInForce != TimeInForce.GoodTilCancelled)
+            {
+                message = new BrokerageMessageEvent(BrokerageMessageType.Warning, "NotSupported",
+                    "This model does not support " + order.TimeInForce + " time in force."


awesomely perfect and simple, great stuff!

mchandschuh · 2018-05-01T13:40:54Z

Common/Brokerages/InteractiveBrokersBrokerageModel.cs

+            if (security.Type != SecurityType.Equity &&
+                security.Type != SecurityType.Forex &&
+                security.Type != SecurityType.Option &&
+                security.Type != SecurityType.Future)


no change required, but typically when there's a set we're comparing against, it's often cleaner to have:

private static readonly HashSet<SecurityType> SupportedSecurityTypes = new HashSet<SecurityType>{ SecurityType.Equity, SecurityType.Forex, SecurityType.Option, SecurityType.Future }; if (!SupportedSecurityTypes.Contains(security.Type) { // error }

mchandschuh · 2018-05-01T13:41:50Z

Common/Brokerages/InteractiveBrokersBrokerageModel.cs

+            }
+
+            // validate time in force
+            if (order.TimeInForce != TimeInForce.GoodTilCancelled && order.TimeInForce != TimeInForce.Day)


Similar comment to the above, often cleaner to define a collection of supported cases and check the collection. This makes the code read nicer and makes it easier to update the conditions in the future as we add/remove support for various things.

mchandschuh · 2018-05-01T13:43:16Z

Common/Interfaces/IAlgorithm.cs

+        /// <summary>
+        /// Gets the market hours database in use by this algorithm
+        /// </summary>
+        MarketHoursDatabase MarketHoursDatabase { get; }


I'm reluctant about this. In theory, this is a great idea, but I have concerns on the direction of the dependency. Currently QCAlgorithm is hard-coded to fetch FromDataFolder() (as is the rest of the application). I love the intent behind this, making things instance based, but if we're going to make accesses to the market hours db instance based we should tackle that in a separate PR and update the solution to follow that new pattern.

mchandschuh · 2018-05-01T13:44:24Z

Common/Interfaces/ITimeInForceHandler.cs

+        /// </summary>
+        /// <param name="order">The order to be checked</param>
+        /// <returns>Returns true if the order has expired, false otherwise</returns>
+        bool HasOrderExpired(Order order);


We spoke about adding the Security instance here to match existing patterns w.r.t models in lean. For example, look at the fee/fill/slippage models, all of which accept the security object in addition to the order object. This will remove the need for the algorithm and market hours db dependencies

- removed IAlgorithm dependency from time in force handlers - renamed GoodTilCancelled to GoodTilCanceled - added GTC time in force in regression algorithms - inlined initialization of time in force handler dictionary

mchandschuh

I think a rebase is preventing me from commenting further, submitting half review.

mchandschuh · 2018-05-01T18:34:55Z

Algorithm/QCAlgorithm.cs

@@ -72,7 +72,7 @@ public partial class QCAlgorithm : MarshalByRefObject, IAlgorithm
        /// <summary>
        /// Gets the market hours database in use by this algorithm
        /// </summary>
-        protected MarketHoursDatabase MarketHoursDatabase { get; }
+        public MarketHoursDatabase MarketHoursDatabase { get; }


Please revert this change.

mchandschuh · 2018-05-01T18:50:15Z

Brokerages/Backtesting/BacktestingBrokerage.cs

+                        // check if the fill should be emitted
+                        if (!timeInForceHandler.IsFillValid(security, order, fill))
+                        {
+                            break;


OK for now, but this will change, we can't assume the implementation of IsFillValid negates all fills in the event there are multiple fills -- when we wire up order event projections this will change, so it's ok for now.

mchandschuh · 2018-05-01T18:53:02Z

Common/Orders/TimeInForces/DayTimeInForceHandler.cs

+                case SecurityType.Forex:
+                case SecurityType.Cfd:
+                    // expires at 5 PM NewYork time
+                    var expiryTime = new DateTime(orderTime.Date.Year, orderTime.Date.Month, orderTime.Date.Day, 17, 0, 0);


we say they expire at 5PM new york but are converting to exchange tz with no guarantee that it's new york time. Is the 5PM represented in the market hours segments? Maybe there's a more general way to define this cut off time? I don't know the answer, but worth considering.

mchandschuh · 2018-05-01T18:53:42Z

Common/Orders/TimeInForces/DayTimeInForceHandler.cs

+                case SecurityType.Cfd:
+                    // expires at 5 PM NewYork time
+                    var expiryTime = new DateTime(orderTime.Date.Year, orderTime.Date.Month, orderTime.Date.Day, 17, 0, 0);
+                    expired = time.Date > orderTime.Date || time.ConvertTo(exchangeHours.TimeZone, TimeZones.NewYork) >= expiryTime;


What if I submit an order at 5:01PM -- is that order good until the next day's 5PM, in which case I think the first part of this condition is incorrect.

mchandschuh

Looks like some previous comments didn't get pushed in the previous review. Upon loading this page the following comments were still marked as pending:

mchandschuh · 2018-05-01T18:56:25Z

Common/Orders/TimeInForces/DayTimeInForceHandler.cs

+                case SecurityType.Future:
+                default:
+                    // expires at market close
+                    expired = time.Date > orderTime.Date || !exchangeHours.IsOpen(time, false);


this might be more generally expressed using exchangeHours.GetNextMarketClose(orderTime, false) and checking if the current time is after the market close following the order time.

mchandschuh · 2018-05-01T18:57:30Z

Common/Orders/TimeInForces/GoodTilCanceledTimeInForceHandler.cs

+        /// <param name="security">The security matching the order</param>
+        /// <param name="order">The order to be checked</param>
+        /// <returns>Returns true if the order has expired, false otherwise</returns>
+        public bool HasOrderExpired(Security security, Order order)


nit - we use Is terminology below but Has terminology here. Consider changing to IsOrderExpired to match verbiage from IsFillValid

mchandschuh · 2018-05-01T18:58:06Z

Tests/RegressionTests.cs

+            {
+                {"Total Trades", "1"},
+                {"Average Win", "0%"},
+                {"Average Loss", "0%"},


Perhaps we can modify the algorithm to get these to be non-zero.

mchandschuh

I think the only required changes here are to use GetNextMarketClose instead of assuming a 5PM closing time. The nice part about using existing abstractions is that if hours get updated (for example holiday hours) then this code will just work and will not be tightly coupled to the assumption that every trading day ends at 5PM

mchandschuh · 2018-05-02T18:30:09Z

Brokerages/Backtesting/BacktestingBrokerage.cs

+            { TimeInForce.GoodTilCanceled, new GoodTilCanceledTimeInForceHandler() },
+            { TimeInForce.Day, new DayTimeInForceHandler() },
+            // Custom time in force will be renamed to GTD soon and will have its own new handler
+            { TimeInForce.Custom, new GoodTilCanceledTimeInForceHandler() }


This looks nice and clean! With the upcoming refactor this is going to change (again) such that the implementation will ride along w/ the order object -- but for now it's nice a clear what the mappings are.

mchandschuh · 2018-05-02T18:31:43Z

Common/Orders/TimeInForces/DayTimeInForceHandler.cs

+                        expiryTime = expiryTime.AddDays(1);
+                    }
+
+                    expired = time.ConvertTo(exchangeHours.TimeZone, cutOffTimeZone) >= expiryTime;


as a reminder, we discussed using exchangeHours.GetNextMarketClose(orderTime, false) to determine when the end of 'day' should be.

It looks like exchangeHours.GetNextMarketClose(orderTime, false) with Forex market hours will only work on Friday (the only day with a market close at 5PM), on the other weekdays it will return midnight.

you're 100% correct, it won't work that way -- let's keep it as you have it. If we ever need a more dynamic value we can look into sourcing the value through the brokerage model.

StefanoRaggi added 8 commits April 30, 2018 09:28

Add TimeInForce.Day support to IB brokerage

8367b6b

Add ITimeInForceHandler interface with GTC and DAY implementations

48c156e

Add time in force handler calls in BacktestingBrokerage

f62c04c

Rename ITimeInForceHandler methods

66d6de1

Add base OrderProperties class

637c153

Add unit tests for time in force handlers

d1b244d

Add message to order event when time in force expired

ab82d99

Add TimeInForceAlgorithm regression tests

38ac2c5

mchandschuh suggested changes Apr 30, 2018

View reviewed changes

Initialize time in force handlers in backtesting brokerage constructors

1ecf9db

mchandschuh suggested changes May 1, 2018

View reviewed changes

Review TimeInForce handler interface

2d19ab7

- removed IAlgorithm dependency from time in force handlers - renamed GoodTilCancelled to GoodTilCanceled - added GTC time in force in regression algorithms - inlined initialization of time in force handler dictionary

StefanoRaggi force-pushed the feature-1093-ib-time-in-force-day branch from f186c26 to 2d19ab7 Compare May 1, 2018 18:54

mchandschuh reviewed May 1, 2018

View reviewed changes

Fix time zone bug with Forex orders in DayTimeInForceHandler

bfe5a8b

mchandschuh reviewed May 2, 2018

View reviewed changes

mchandschuh suggested changes May 2, 2018

View reviewed changes

StefanoRaggi added 2 commits May 2, 2018 21:00

Rename HasOrderExpired to IsOrderExpired

a85ebe4

Update DayTimeInForceHandler to use GetNextMarketClose

62e1c2e

mchandschuh approved these changes May 3, 2018

View reviewed changes

mchandschuh merged commit d878857 into QuantConnect:master May 3, 2018

StefanoRaggi deleted the feature-1093-ib-time-in-force-day branch May 3, 2018 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TimeInForce.Day support in backtesting and IB brokerage #1938

Add TimeInForce.Day support in backtesting and IB brokerage #1938

StefanoRaggi commented Apr 30, 2018

mchandschuh left a comment

mchandschuh Apr 30, 2018

mchandschuh Apr 30, 2018

mchandschuh Apr 30, 2018

mchandschuh Apr 30, 2018

StefanoRaggi Apr 30, 2018

mchandschuh Apr 30, 2018

mchandschuh left a comment

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh left a comment

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh left a comment

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh May 1, 2018

mchandschuh left a comment

mchandschuh May 2, 2018

mchandschuh May 2, 2018

StefanoRaggi May 2, 2018

mchandschuh May 3, 2018


		var time = _algorithm.UtcTime.ConvertFromUtc(exchangeHours.TimeZone);

		return !exchangeHours.IsOpen(time, false);

Add TimeInForce.Day support in backtesting and IB brokerage #1938

Add TimeInForce.Day support in backtesting and IB brokerage #1938

Conversation

StefanoRaggi commented Apr 30, 2018

Description

Related Issue

Motivation and Context

Requires Documentation Change

How Has This Been Tested?

Types of changes

Checklist:

mchandschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchandschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchandschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchandschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchandschuh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment