Fix fill forward resolution adjustment on removal (QuantConnect#8049)

* Fix fill forward resolution adjustment on removal - Fix fill forward resolution adjustment on subscription removal. Adding regression algorithm reproducing issue * Minor fix for undeterminism behavior on security removals and FF res change * Further improvements and determinism - Improve regression algorithm add, remove and readd future, asserting FF resolution changes - Adjust universe selection to be deterministic and avoid unnecessary FF resolution changes
lixin34 · May 24, 2024 · 3a99e0b · 3a99e0b
1 parent 5f3cd20
commit 3a99e0b
Show file tree

Hide file tree

Showing 9 changed files with 191 additions and 22 deletions.
diff --git a/Algorithm.CSharp/AllShortableSymbolsCoarseSelectionRegressionAlgorithm.cs b/Algorithm.CSharp/AllShortableSymbolsCoarseSelectionRegressionAlgorithm.cs
@@ -238,7 +238,7 @@ public Dictionary<Symbol, long> AllShortableSymbols(DateTime localTime)
         /// <summary>
         /// Data Points count of all timeslices of algorithm
         /// </summary>
-        public long DataPoints => 37754;
+        public long DataPoints => 37751;
 
         /// <summary>
         /// Data Points count of the algorithm history

diff --git a/Algorithm.CSharp/FillForwardResolutionAdjustedOnRemovalRegressionAlgorithm.cs b/Algorithm.CSharp/FillForwardResolutionAdjustedOnRemovalRegressionAlgorithm.cs
@@ -0,0 +1,135 @@
+/*
+ * QUANTCONNECT.COM - Democratizing Finance, Empowering Individuals.
+ * Lean Algorithmic Trading Engine v2.0. Copyright 2014 QuantConnect Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+*/
+
+using System;
+using System.Collections.Generic;
+using QuantConnect.Data;
+using QuantConnect.Interfaces;
+
+namespace QuantConnect.Algorithm.CSharp
+{
+    /// <summary>
+    /// Regression algorithm reproduces GH issue #8023, where fill forward resolution will no get updated in some cases on security removals
+    /// </summary>
+    public class FillForwardResolutionAdjustedOnRemovalRegressionAlgorithm : QCAlgorithm, IRegressionAlgorithmDefinition
+    {
+        private readonly Queue<DateTime> _expectedDataTimes = new(new DateTime[]
+        {
+            new DateTime(2013, 10, 7, 9, 31, 0),
+            new DateTime(2013, 10, 8, 0, 0, 0),
+            new DateTime(2013, 10, 9, 0, 0, 0),
+            new DateTime(2013, 10, 9, 0, 0, 0), // TODO
+            new DateTime(2013, 10, 9, 9, 31, 0),
+            new DateTime(2013, 10, 10, 0, 0, 0),
+            new DateTime(2013, 10, 11, 0, 0, 0)
+        });
+
+        public override void Initialize()
+        {
+            SetStartDate(2013, 10, 07);
+            SetEndDate(2013, 10, 10);
+
+            AddFuture("ES", Resolution.Minute);
+            AddEquity("SPY", Resolution.Daily);
+        }
+
+        /// <summary>
+        /// OnData event is the primary entry point for your algorithm. Each new data point will be pumped in here.
+        /// </summary>
+        /// <param name="data">Slice object keyed by symbol containing the stock data</param>
+        public override void OnData(Slice data)
+        {
+            if (_expectedDataTimes.Dequeue() != Time)
+            {
+                throw new Exception($"Unexpected data time {Time}!");
+            }
+
+            if (ActiveSecurities.ContainsKey("/ES"))
+            {
+                if (data.ContainsKey("/ES"))
+                {
+                    RemoveSecurity("/ES");
+                }
+            }
+            else if (Time == new DateTime(2013, 10, 9, 0, 0, 0))
+            {
+                // let's re add it
+                AddFuture("ES", Resolution.Minute);
+            }
+        }
+
+        public override void OnEndOfAlgorithm()
+        {
+            if (_expectedDataTimes.Count != 0)
+            {
+                throw new Exception($"OnEndOfAlgorithm(): Unexpected data times!");
+            }
+        }
+
+        /// <summary>
+        /// This is used by the regression test system to indicate if the open source Lean repository has the required data to run this algorithm.
+        /// </summary>
+        public bool CanRunLocally { get; } = true;
+
+        /// <summary>
+        /// This is used by the regression test system to indicate which languages this algorithm is written in.
+        /// </summary>
+        public Language[] Languages { get; } = { Language.CSharp };
+
+        /// <summary>
+        /// Data Points count of all timeslices of algorithm
+        /// </summary>
+        public long DataPoints => 96;
+
+        /// <summary>
+        /// Data Points count of the algorithm history
+        /// </summary>
+        public int AlgorithmHistoryDataPoints => 0;
+
+        /// <summary>
+        /// This is used by the regression test system to indicate what the expected statistics are from running the algorithm
+        /// </summary>
+        public Dictionary<string, string> ExpectedStatistics => new Dictionary<string, string>
+        {
+            {"Total Orders", "0"},
+            {"Average Win", "0%"},
+            {"Average Loss", "0%"},
+            {"Compounding Annual Return", "0%"},
+            {"Drawdown", "0%"},
+            {"Expectancy", "0"},
+            {"Start Equity", "100000"},
+            {"End Equity", "100000"},
+            {"Net Profit", "0%"},
+            {"Sharpe Ratio", "0"},
+            {"Sortino Ratio", "0"},
+            {"Probabilistic Sharpe Ratio", "0%"},
+            {"Loss Rate", "0%"},
+            {"Win Rate", "0%"},
+            {"Profit-Loss Ratio", "0"},
+            {"Alpha", "0"},
+            {"Beta", "0"},
+            {"Annual Standard Deviation", "0"},
+            {"Annual Variance", "0"},
+            {"Information Ratio", "-5.677"},
+            {"Tracking Error", "0.271"},
+            {"Treynor Ratio", "0"},
+            {"Total Fees", "$0.00"},
+            {"Estimated Strategy Capacity", "$0"},
+            {"Lowest Capacity Asset", ""},
+            {"Portfolio Turnover", "0%"},
+            {"OrderListHash", "d41d8cd98f00b204e9800998ecf8427e"}
+        };
+    }
+}
diff --git a/Algorithm.CSharp/RegressionAlgorithm.cs b/Algorithm.CSharp/RegressionAlgorithm.cs
@@ -83,7 +83,7 @@ public override void OnData(Slice data)
         /// <summary>
         /// Data Points count of all timeslices of algorithm
         /// </summary>
-        public long DataPoints => 16896626;
+        public long DataPoints => 16896627;
 
         /// <summary>
         /// Data Points count of the algorithm history
@@ -95,17 +95,17 @@ public override void OnData(Slice data)
         /// </summary>
         public Dictionary<string, string> ExpectedStatistics => new Dictionary<string, string>
         {
-            {"Total Orders", "1592"},
+            {"Total Orders", "1593"},
             {"Average Win", "0.00%"},
             {"Average Loss", "0.00%"},
-            {"Compounding Annual Return", "-1.257%"},
+            {"Compounding Annual Return", "-1.158%"},
             {"Drawdown", "0.000%"},
             {"Expectancy", "-0.989"},
             {"Start Equity", "10000000"},
-            {"End Equity", "9998382.41"},
+            {"End Equity", "9998403.92"},
             {"Net Profit", "-0.016%"},
-            {"Sharpe Ratio", "-18.139"},
-            {"Sortino Ratio", "-18.139"},
+            {"Sharpe Ratio", "-18.339"},
+            {"Sortino Ratio", "-18.339"},
             {"Probabilistic Sharpe Ratio", "0.000%"},
             {"Loss Rate", "100%"},
             {"Win Rate", "0%"},
@@ -114,14 +114,14 @@ public override void OnData(Slice data)
             {"Beta", "-0.001"},
             {"Annual Standard Deviation", "0.001"},
             {"Annual Variance", "0"},
-            {"Information Ratio", "-8.944"},
+            {"Information Ratio", "-8.943"},
             {"Tracking Error", "0.223"},
-            {"Treynor Ratio", "27.031"},
-            {"Total Fees", "$1587.00"},
-            {"Estimated Strategy Capacity", "$64000.00"},
-            {"Lowest Capacity Asset", "IBM R735QTJ8XC9X"},
-            {"Portfolio Turnover", "1.86%"},
-            {"OrderListHash", "56c4f2806983e52ef5087606512e2844"}
+            {"Treynor Ratio", "27.123"},
+            {"Total Fees", "$1589.00"},
+            {"Estimated Strategy Capacity", "$67000000.00"},
+            {"Lowest Capacity Asset", "AIG R735QTJ8XC9X"},
+            {"Portfolio Turnover", "1.87%"},
+            {"OrderListHash", "2efcb91611db79040f62f0f5c36280c9"}
         };
     }
 }
diff --git a/Engine/DataFeeds/DataManager.cs b/Engine/DataFeeds/DataManager.cs
@@ -210,13 +210,20 @@ public DataManager(
                         // remove
                         foreach (var request in requests)
                         {
-                            RemoveSubscription(request.Configuration, request.Universe);
+                            // force because we want them actually removed even if still a member of the universe, because the FF res changed
+                            // which means we will drop any data points that could be in the next potential slice being created
+                            RemoveSubscriptionInternal(request.Configuration, universe: request.Universe, forceSubscriptionRemoval: true);
                         }
 
                         // re add
                         foreach (var request in requests)
                         {
-                            AddSubscription(new SubscriptionRequest(request, startTimeUtc: algorithm.UtcTime));
+                            // TODO: If it is an add we will set time 1 tick ahead to properly sync data
+                            // with next timeslice, avoid emitting now twice.
+                            // We do the same in the 'TimeTriggeredUniverseSubscriptionEnumeratorFactory' when handling changes
+                            AddSubscription(new SubscriptionRequest(request, startTimeUtc: algorithm.UtcTime
+                                //.AddTicks(1) TODO
+                                ));
                         }
 
                         DataFeedSubscriptions.FreezeFillForwardResolution(false);
@@ -321,6 +328,19 @@ public bool AddSubscription(SubscriptionRequest request)
         /// Default value, null, will remove all universes</param>
         /// <returns>True if the subscription was successfully removed, false otherwise</returns>
         public bool RemoveSubscription(SubscriptionDataConfig configuration, Universe universe = null)
+        {
+            return RemoveSubscriptionInternal(configuration, universe, forceSubscriptionRemoval: false);
+        }
+
+        /// <summary>
+        /// Removes the <see cref="Subscription"/>, if it exists
+        /// </summary>
+        /// <param name="configuration">The <see cref="SubscriptionDataConfig"/> of the subscription to remove</param>
+        /// <param name="universe">Universe requesting to remove <see cref="Subscription"/>.
+        /// Default value, null, will remove all universes</param>
+        /// <param name="forceSubscriptionRemoval">We force the subscription removal by marking it as removed from universe, so that all it's data is dropped</param>
+        /// <returns>True if the subscription was successfully removed, false otherwise</returns>
+        private bool RemoveSubscriptionInternal(SubscriptionDataConfig configuration, Universe universe, bool forceSubscriptionRemoval)
         {
             // remove the subscription from our collection, if it exists
             Subscription subscription;
@@ -347,6 +367,11 @@ public bool RemoveSubscription(SubscriptionDataConfig configuration, Universe un
 
                     RemoveSubscriptionDataConfig(subscription);
 
+                    if (forceSubscriptionRemoval)
+                    {
+                        subscription.MarkAsRemovedFromUniverse();
+                    }
+
                     if (_liveMode)
                     {
                         Log.Trace($"DataManager.RemoveSubscription(): Removed {configuration}");

diff --git a/Engine/DataFeeds/Enumerators/Factories/TimeTriggeredUniverseSubscriptionEnumeratorFactory.cs b/Engine/DataFeeds/Enumerators/Factories/TimeTriggeredUniverseSubscriptionEnumeratorFactory.cs
@@ -72,7 +72,8 @@ public IEnumerator<BaseData> CreateEnumerator(SubscriptionRequest request, IData
                 universe.CollectionChanged += (sender, args) =>
                 {
                     // If it is an add we will set time 1 tick ahead to properly sync data
-                    // with next timeslice, if it is a remove then we will set time to now
+                    // with next timeslice, avoid emitting now twice, if it is a remove then we will set time to now
+                    // we do the same in the 'DataManager' when handling FF resolution changes
                     IList items;
                     DateTime time;
                     if (args.Action == NotifyCollectionChangedAction.Add)

diff --git a/Engine/DataFeeds/SubscriptionCollection.cs b/Engine/DataFeeds/SubscriptionCollection.cs
@@ -102,7 +102,12 @@ public bool TryRemove(SubscriptionDataConfig configuration, out Subscription sub
         {
             if (_subscriptions.TryRemove(configuration, out subscription))
             {
-                UpdateFillForwardResolution(FillForwardResolutionOperation.AfterRemove, configuration);
+                // for user friendlyness only look at removals triggerd by the user not those that are due to a data feed ending because of no more data,
+                // let's try to respect the users original FF enumerator request
+                if (!subscription.EndOfStream)
+                {
+                    UpdateFillForwardResolution(FillForwardResolutionOperation.AfterRemove, configuration);
+                }
                 _sortingSubscriptionRequired = true;
                 return true;
             }
@@ -179,7 +184,8 @@ private void UpdateFillForwardResolution(FillForwardResolutionOperation operatio
                 ||
                     (operation == FillForwardResolutionOperation.AfterRemove // We are removing
                     && configuration.Increment == _fillForwardResolution.Value // True: We are removing the resolution we were using
-                    && _subscriptions.All(x => x.Key.Resolution != configuration.Resolution))) // False: there is at least another one equal, no need to update
+                    // False: there is at least another one equal, no need to update, but we only look at those valid configuration which are the ones which set the FF resolution
+                    && _subscriptions.Keys.All(x => !ValidateFillForwardResolution(x) || x.Resolution != configuration.Resolution)))
                 )
             {
                 var configurations = (operation == FillForwardResolutionOperation.BeforeAdd)

diff --git a/Engine/DataFeeds/SubscriptionSynchronizer.cs b/Engine/DataFeeds/SubscriptionSynchronizer.cs
@@ -223,7 +223,9 @@ public IEnumerable<TimeSlice> Sync(IEnumerable<Subscription> subscriptions,
                         // time pulse to align algorithm time with current frontier
                         yield return _timeSliceFactory.CreateTimePulse(frontierUtc);
 
-                        foreach (var kvp in universeData)
+                        // trigger the smalled resolution first, so that FF res get's set once from the start correctly
+                        // while at it, let's make it determininstic and sort by universe sid later
+                        foreach (var kvp in universeData.OrderBy(x => x.Key.Configuration.Resolution).ThenBy(x => x.Key.Symbol.ID))
                         {
                             var universe = kvp.Key;
                             var baseDataCollection = kvp.Value;

diff --git a/Engine/DataFeeds/UniverseSelection.cs b/Engine/DataFeeds/UniverseSelection.cs
@@ -200,7 +200,7 @@ public SecurityChanges ApplyUniverseSelection(Universe universe, DateTime dateTi
             }
 
             // determine which data subscriptions need to be removed from this universe
-            foreach (var member in universe.Securities.Values.OrderBy(member => member.Security.Symbol.SecurityType))
+            foreach (var member in universe.Securities.Values.OrderBy(member => member.Security.Symbol.SecurityType).ThenBy(x => x.Security.Symbol.ID))
             {
                 var security = member.Security;
                 // if we've selected this subscription again, keep it

diff --git a/Tests/Engine/DataFeeds/LiveTradingDataFeedTests.cs b/Tests/Engine/DataFeeds/LiveTradingDataFeedTests.cs
@@ -3706,7 +3706,7 @@ public void HandlesFutureAndOptionChainUniverse(SecurityType securityType)
             }
             else if (securityType == SecurityType.Future)
             {
-                var future = algorithm.AddFuture(Futures.Indices.SP500EMini, Resolution.Minute, extendedMarketHours: true);
+                var future = algorithm.AddFuture(Futures.Indices.SP500EMini, Resolution.Minute, extendedMarketHours: true, fillForward: false);
                 // Must include weeklys because the contracts returned by the lookup, futureSymbol1 & futureSymbol2, are non-standard
                 future.SetFilter(x => x.IncludeWeeklys());
                 exchangeTimeZone = future.Exchange.TimeZone;