Add TimeInForce.GoodTilDate (GTD) support in backtesting and IB brokerage #2032

StefanoRaggi · 2018-05-24T11:20:52Z

Description

This PR includes some refactoring to make it easier to add new time in forces (with custom settings, such as Expiry property in GoodTilDate):

Renamed TimeInForce enum to TimeInForceType
Added new TimeInForce abstract class
Removed ITimeInForceHandler implementations, existing code moved to the derived TimeInForce classes
Removed Order.DurationValue property, replaced by GoodTilDateTimeInForce.Expiry
BacktestingBrokerage no longer requires a dictionary of time in force handlers, it now directly calls methods on TimeInForce objects

Unit tests have been added and example usage has been added to the existing TimeInForceAlgorithm.

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

Related Issue

QuantConnect/Lean.Brokerages.InteractiveBrokers#27

Motivation and Context

Good Til Date (GTD) time in force was not currently supported

Requires Documentation Change

No.

How Has This Been Tested?

New unit tests, updated 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

Lots of comments but overall very great stuff :) I love seeing the design we initially laid out naturally grow and evolve. Main comments relate to equality operators and implementations in the base class and questions around whether or not we still need TimeInForceType since TimeInForce supplies the same members and the types are already embodied via the individual TIF implementation types.

mchandschuh · 2018-05-25T13:36:28Z

Algorithm.CSharp/TimeInForceAlgorithm.cs

+                // This order will expire on October 10th at market close,
+                // if not filled by then it will be canceled automatically.
+
+                DefaultOrderProperties.TimeInForce = new GoodTilDateTimeInForce(new DateTime(2013, 10, 10));


I'm wondering if it makes more sense to define GoodTilDateTimeInForce using a TimeSpan instead of an actual DateTime. Using a time span I think is more natural and also doesn't require it to be updated every day. Also, from an implementation standpoint it's easily supported since the order object has the submit time on it, so can easily compute the date we should cancel. Thoughts?

Also, since this is under regression test, maybe we should have a case where GTD is cancelled and GTD gets executed. Also, let's confirm that other TIF have both sides exercised in the regression algorithm, otherwise we're only testing half of the implementation via regression.

Another reason here, I'm thinking of eventually supporting syntax like:

DefaultOrderProperties.TimeInForce = TimeSpan.FromDays(10);

via an implicit operator which would require the data held to be a time span value instead of an absolute date time value.

The only issue we would have changing GTD from DateTime Expiry to TimeSpan Duration is that not all brokerage API have the order submission time available, so we would not be able to get back the Duration value in GetOpenOrders.

IB and FXCM do not have order creation/submission time in their get order API methods while OANDA and GDAX have this field.

IB, FXCM and OANDA have the expiry date/time field, GDAX doesn't have it but in its GTT time in force only allows fixed durations of 1 minute, 1 hour or 1 day.

Maybe we need to have both properties?

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

Brokerages/InteractiveBrokers/InteractiveBrokersBrokerage.cs

+            //   when reading the open orders, the same property will have this value: "20180524 23:00:00 CET"
+            //   which is incorrect: should be CEST (UTC+2) instead of CET (UTC+1)
+
+            // We can ignore this issue, because the method is only called by GetOpenOrders.


great comment above about IB's mishandling of time zones, perhaps add an additional blurb to the bottom line here. why is it OK that it's only called by GetOpenOrders? -- my hunch is that it's because we only call GetOpenOrders during live trading, which means we won't be simulating TIF and instead will rely on brokerages to apply TIF properly.

it's because we only call GetOpenOrders during live trading, which means we won't be simulating TIF and instead will rely on brokerages to apply TIF properly.
This is exactly the reason -- I'll add this comment to make it more clear

mchandschuh · 2018-05-25T13:45:43Z

Common/Orders/OrderJsonConverter.cs

+            }
+
+            var timeInForceType = (TimeInForceType)timeInForce["Type"].Value<int>();
+            switch (timeInForceType)


maybe this switch statement can be removed/simplified by ensuring that we're serializing the $type to JSON. This will reduce coupling within the system and prevent potential errors from changes/additions to the TIF implementations.

I'll try to handle serialization/deserialization with $type, although we'll still need a switch on a few numeric values (0,1,2) for backwards-compatibility.

mchandschuh · 2018-05-25T13:48:35Z

Common/Orders/TimeInForce.cs

+                    return new GoodTilCanceledTimeInForce();
+
+                case TimeInForceType.Day:
+                    return new DayTimeInForce();


Instead of creating a new object, can we use the existing static readonly instances already defined? IIRC, all of these supported implementations are stateless.

Sure, this was an oversight

mchandschuh · 2018-05-25T13:49:41Z

Common/Orders/TimeInForce.cs

+            if (ReferenceEquals(left, null)) return false;
+            if (ReferenceEquals(right, null)) return false;
+
+            return left.Type == right.Type;


This isn't 100% correct. Consider two GTD implementations with different end dates (or different time spans if we make that change). Best bet is to return left.Equals(right) and let the implementations decide how to resolve equality. Similar comment for all equality methods in this class.

The idea here was to have a common default implementation of equality methods (to avoid duplicating these methods in derived classes with no additional properties -- such as GTC and DAY) , but since I'm going to remove the TimeInForceType Type property, this will be changed to left.Equals(right).

mchandschuh · 2018-05-25T13:53:01Z

Common/Orders/OrderTypes.cs

    /// </summary>
-    public enum TimeInForce
+    public enum TimeInForceType


Is this enum still required? I think the only place it's used anymore is type checks which can be replaced by actual type checks vs enum value checks. The other place it's currently used is the implicit operator, but I don't think that one is required since users can use TimeInForce.Day instead of TimeInForceType.Day -- the concern is having two very similar things that are potentially different.

👍 Agree, the TimeInForceType enum is not really needed any more -- I'll remove it

mchandschuh · 2018-05-25T14:00:13Z

Tests/Common/Orders/TimeInForces/TimeInForceTests.cs

+            var tif3 = new GoodTilDateTimeInForce(new DateTime(2018, 6, 6));
+
+            Assert.IsTrue(tif1 == tif2);
+            Assert.IsFalse(tif1 != tif2);


I suspect this test will unexpectedly fail if changed to:

TimeInForce tif1 = new GoodTilDateTimeInForce(new DateTime(2018, 5, 21)); TimeInForce tif2 = new GoodTilDateTimeInForce(new DateTime(2018, 5, 21)); TimeInForce tif3 = new GoodTilDateTimeInForce(new DateTime(2018, 6, 6)); Assert.IsTrue(tif1 == tif2); Assert.IsFalse(tif1 != tif2);

This is because the static equals operator does not support polymorphic/dynamic dispatch, so it will bind to the equals operator defined in TimeInForce (the one statically known) instead of the one defined in GoodTilDateTimeInForce -- see comments in TimeInForce regarding equality implementation.

Aren't these asserts currently passing in the PR?
All equality methods and operators have been overridden in the GoodTilDateTimeInForce -- will be refactored shortly

mchandschuh · 2018-05-25T14:01:58Z

Common/Orders/TimeInForce.cs

+        /// <returns>A hash code for the current <see cref="TimeInForce"/>.</returns>
+        public override int GetHashCode()
+        {
+            return Type.GetHashCode();


The more I think about equality implementations the more I think these should be defined as abstract or not defined at all in this class (Equals and GetHashCode) and the operators == and != should pipe through to the Equals method to enable proper dynamic dispatch. Thoughts?

Agree on the abstract equality methods and dynamic dispatch, if possible I'd try leaving the == and != in the base class (to avoid duplication)

This property has been moved to GoodTilDateTimeInForce.Expiry

- removed TimeInForceType enum - updated comments in InteractiveBrokersBrokerage.ParseExpiryDateTime - removed equality methods and operators from TimeInForce class - fixed TimeInForce JSON serialization/deserialization with custom converter

- Added TimeInForce.GoodTilDate static method - Made GoodTilDateTimeInForce.Expiry readonly

mchandschuh

Awesome stuff, thanks @StefanoRaggi!

mchandschuh · 2018-05-30T15:18:53Z

Common/Orders/TimeInForce.cs

+    /// Time In Force - defines the length of time over which an order will continue working before it is canceled
+    /// </summary>
+    [JsonConverter(typeof(TimeInForceJsonConverter))]
+    public abstract class TimeInForce : ITimeInForceHandler


Awesome!! thanks!

mchandschuh · 2018-05-30T15:20:59Z

Common/Orders/TimeInForceJsonConverter.cs

+                }
+            }
+
+            jo.WriteTo(writer);


Interesting, I figured this would have been simpler, somehow making use of JsonSerializerSettings.TypeNameHandling -- but this is simple too!

mchandschuh suggested changes May 25, 2018

View reviewed changes

StefanoRaggi force-pushed the feature-1093-timeinforce-gtd branch from 8f5d582 to 0062f54 Compare May 25, 2018 23:09

StefanoRaggi added 9 commits May 30, 2018 10:00

Refactor TimeInForce enum to class

9ae346b

Make TimeInForce class abstract

4406332

Add Good Til Date (GTD) time in force

9116531

Remove Order.DurationValue property

59ee1d2

This property has been moved to GoodTilDateTimeInForce.Expiry

Add GTD time in force support in IB brokerage

d38fbba

Add GTD sample code in TimeInForceAlgorithm

4b2cc43

Fix IB issues with GTD orders

bbf3e32

Post review updates

7a76b07

- removed TimeInForceType enum - updated comments in InteractiveBrokersBrokerage.ParseExpiryDateTime - removed equality methods and operators from TimeInForce class - fixed TimeInForce JSON serialization/deserialization with custom converter

Added more orders in TimeInForceAlgorithm regression

1fea8b8

StefanoRaggi force-pushed the feature-1093-timeinforce-gtd branch from 0062f54 to 1fea8b8 Compare May 30, 2018 10:00

Minor refactor

8613deb

- Added TimeInForce.GoodTilDate static method - Made GoodTilDateTimeInForce.Expiry readonly

mchandschuh approved these changes May 30, 2018

View reviewed changes

Remove unnecessary binding flags in TimeInForceJsonConverter

ce51fcb

mchandschuh merged commit d2ac77b into QuantConnect:master May 30, 2018

StefanoRaggi deleted the feature-1093-timeinforce-gtd branch May 30, 2018 20:22

mchandschuh mentioned this pull request Jul 6, 2018

live-interactive mode: Ability to accept GoodTillDate and DurationValue in QC orders for IB #971

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TimeInForce.GoodTilDate (GTD) support in backtesting and IB brokerage #2032

Add TimeInForce.GoodTilDate (GTD) support in backtesting and IB brokerage #2032

StefanoRaggi commented May 24, 2018

mchandschuh left a comment

mchandschuh May 25, 2018

mchandschuh May 25, 2018

mchandschuh May 25, 2018

StefanoRaggi May 28, 2018

mchandschuh May 25, 2018

StefanoRaggi May 25, 2018

mchandschuh May 25, 2018

StefanoRaggi May 25, 2018

mchandschuh May 25, 2018

StefanoRaggi May 25, 2018

mchandschuh May 25, 2018

StefanoRaggi May 25, 2018

mchandschuh May 25, 2018

StefanoRaggi May 25, 2018

mchandschuh May 25, 2018

StefanoRaggi May 25, 2018

mchandschuh May 25, 2018

StefanoRaggi May 25, 2018

mchandschuh left a comment

mchandschuh May 30, 2018

mchandschuh May 30, 2018

Add TimeInForce.GoodTilDate (GTD) support in backtesting and IB brokerage #2032

Add TimeInForce.GoodTilDate (GTD) support in backtesting and IB brokerage #2032

Conversation

StefanoRaggi commented May 24, 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

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

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