Minute to hour transformation: failure

[pavlexander]

Current version of trady (3.2.9?) does not do the minute to hour transformation.

First of all - following part of code in IOhlcvDataExtension was changed previously:

        var tempCandles = new List<IOhlcv>();
        for (int i = 0; i < orderedCandles.Count; i++)
        {
            var indexTime = orderedCandles[i].DateTime;
            if (indexTime >= periodEndTime)
            {
                periodStartTime = indexTime; // previously: periodEndTime
                periodEndTime = periodInstance.NextTimestamp(periodStartTime);

                AddComputedCandleToOutput(outputCandles, tempCandles);
                tempCandles = new List<IOhlcv>();
            }

            tempCandles.Add(orderedCandles[i]);
        }

The code with the old periodEndTime somehow works better :) After transformation you actually get smaller amount of candles. With indexTime you get the same exactly amount of candles as you had before the transformation. This is issue 1.

So, the problem I am describing next - is actually using the old version of code (with periodStartTime = periodEndTime). You can skip it, if issue 1 is not an issue but a feature?:

        string dataString = File.ReadAllText("data.txt");
        var data = JsonSerializer.Deserialize<List<Candle>>(dataString);
        var transformedTradyCandles = data.Transform<PerMinute, Hourly>();

data.txt
p.s. In order for deserialization to work - you have to have argument-less Candle constructor.

The data is that I passing to transformation is in time frame: 8:40 - 10:02, total of 83 records.

Expected result - 3 candles:

• at 9:00 (aggregated 8:40 - 9:00)
• at 10:00 (aggregated 9:01 - 10:00)
• at 11:00 (aggregated 10:01 - 10:03)

Actual result - 4 candles, wrong time components:

• 8:40
• 8:41
• 9:00
• 10:00

I have not actually compared the amount of Volume. But from the looks of it - candles already have incorrect time component.. This is issue 2

So, as I mentioned, you can skip issue 2 if issue 1 is the one that needs fixing. In either way - you can use the data that I have provided and the sample code to verify either of those 2 issues.

using VS 2019, Current version of trady (3.2.9?, i.e. master branch), .Net core 3.1

[pavlexander]

After some digging through the code I see at least 1 issue that could lead to above problems. In IOhlcvDataExtension:

        var periodStartTime = orderedCandles[0].DateTime;
        var periodEndTime = periodInstance.NextTimestamp(periodStartTime);

The date that is a start date is in format: {12/2/2017 8:40:00 PM +00:00}
And the end date is: {12/2/2017 9:00:00 PM +02:00}
According to how I understand it - the framework should have just added 1 period to start datetime, i.e. 1 hour. But in addition to adding 1 hour + it also changed the timezone.

EDIT: it seems like that after getting the DateTime from DateTimeOffset - the date comes with unspecified kind. So afterwards after the transformations - the Trady at some points adds +2 hours to the offset. I have fixed the above issue forcefully setting the kind to Utc:

    protected override DateTimeOffset ComputeTimestampByCorrectedPeriodCount(DateTimeOffset dateTime, int correctedPeriodCount)
    {
        var sth = TimeSpan.FromSeconds(NumberOfSecond);
        var sth2 = DateTime.SpecifyKind(dateTime.DateTime.Truncate(sth), DateTimeKind.Utc); // FIX here
        var sth3 = sth2.AddSeconds(correctedPeriodCount * NumberOfSecond);

        return sth3;
    }

but it did not resolve the problem completely, though I have to admit - after transformation number of candles decreased from 83 to just 3 - using the new code (periodStartTime = indexTime).

New actual result

• 8:40
• 9:00
• 10:00

So, something is still not completely right :)

Also I just found this:
https://docs.microsoft.com/en-us/dotnet/standard/datetime/converting-between-datetime-and-offset

The DateTime property is most commonly used to perform DateTimeOffset to DateTime conversion. However, it returns a DateTime value whose Kind property is Unspecified, as the following example illustrates.

This means that any information about the DateTimeOffset value's relationship to UTC is lost by the conversion when the DateTime property is used. This affects DateTimeOffset values that correspond to UTC time or to the system's local time because the DateTime structure reflects only those two time zones in its Kind property.

To preserve as much time zone information as possible when converting a DateTimeOffset to a DateTime value, you can use the DateTimeOffset.UtcDateTime and DateTimeOffset.LocalDateTime properties.

so alternative solution (and a better one) would be to use DateTimeOffset.UtcDateTime, for example. You have to evaluate if this change also affects other parts of the Trady framework.

[pavlexander]

Alright, a question:

• Is the a reason why after the transformation - we take the first candle date-time for the resulting candle datetime? Isn't a candle - a summary of the previous period? So in that sense - candle date-tome is always the latest datetime, not the datetime of the first candle...
  In either case - I don't think that earliest candle datetime shall be used. It's either the period start or the end :) Convince me otherwise.

With that idea in mind - I have applied some fixes to the code:

    public static IReadOnlyList<IOhlcv> Transform<TSourcePeriod, TTargetPeriod>(this                 IEnumerable<IOhlcv> candles)
        where TSourcePeriod : IPeriod
        where TTargetPeriod : IPeriod
    {
        // ....

        DateTimeOffset periodStartTime = orderedCandles[0].DateTime;
        DateTimeOffset periodEndTime = periodInstance.NextTimestamp(periodStartTime);

        var tempCandles = new List<IOhlcv>();
        for (int i = 0; i < orderedCandles.Count; i++)
        {
            var indexTime = orderedCandles[i].DateTime;
            if (indexTime > periodEndTime) // FIX. make end period inclusive! (old: >=)
            {
                // CORRECTED order
                AddComputedCandleToOutput(outputCandles, tempCandles, periodEndTime); // CORRECTED
                tempCandles = new List<IOhlcv>();

                periodStartTime = indexTime;
                periodEndTime = periodInstance.NextTimestamp(periodStartTime);
            }

            tempCandles.Add(orderedCandles[i]);
        }

        if (tempCandles.Any())
            AddComputedCandleToOutput(outputCandles, tempCandles, periodEndTime); // CORRECTED

        return outputCandles;
    }

and

    private static void AddComputedCandleToOutput(List<IOhlcv> outputCandles, List<IOhlcv> tempCandles, DateTimeOffset periodEndTime) // CORRECTED
    {
        var computedCandle = ComputeCandles(tempCandles, periodEndTime);
        if (computedCandle != null)
            outputCandles.Add(computedCandle);
    }

and

    private static IOhlcv ComputeCandles(IEnumerable<IOhlcv> candles, DateTimeOffset periodEndTime) // CORRECTED
    {
        if (!candles.Any())
            return null;

        var dateTime = periodEndTime.UtcDateTime; // CORRECTED !!!
        var open = candles.First().Open;
        var high = candles.Max(stick => stick.High);
        var low = candles.Min(stick => stick.Low);
        var close = candles.Last().Close;
        var volume = candles.Sum(stick => stick.Volume);
        return new Candle(dateTime, open, high, low, close, volume);
    }

This, plus the fix applied in previous response:

    protected override DateTimeOffset ComputeTimestampByCorrectedPeriodCount(DateTimeOffset dateTime, int correctedPeriodCount)
    {
        var sth = TimeSpan.FromSeconds(NumberOfSecond);
        var sth2 = dateTime.UtcDateTime.Truncate(sth); // FIX here
        var sth3 = sth2.AddSeconds(correctedPeriodCount * NumberOfSecond);

        return sth3;
    }

New actual result:

• at 9:00 (aggregated 8:40 - 9:00)
• at 10:00 (aggregated 9:01 - 10:00)
• at 11:00 (aggregated 10:01 - 10:03)

sorry for spam. I could have made it a pull request as well. Just interested in your opinion on this topic before any commits..

EDIT: Also, using ElementAt without proper optimizations is many times slower than accessing element by index. If you could change this aspect of Trady - you could increase it's performance tenfold. I had to fix following method:

    private static bool IsTimeframesValid<TPeriod>(IEnumerable<IOhlcv> candles, out IOhlcv err)
        where TPeriod : IPeriod
    {
        var candlesList = candles
            .ToList(); // FIX this

        var periodInstance = Activator.CreateInstance<TPeriod>();
        err = default;
        var offset = candlesList.Any() ? candlesList.First().DateTime.Offset.Hours : 0;
        for (int i = 0; i < candlesList.Count() - 1; i++)
        {
            var chandle = candlesList[i];

            var nextTime = periodInstance.NextTimestamp(chandle.DateTime);
            var candleEndTime = new DateTimeOffset(nextTime.Date, TimeSpan.FromHours(offset));
            if (candleEndTime > candlesList[i + 1].DateTime)
            {
                err = chandle;
                return false;
            }
        }
        return true;
    }

In my case I have 1_232_239 minute ticks. Waited for 1 minute for the transformation to finish on 8700k 4 core CPU... After the fix it takes a second at most. I have not inspected the code, but I suppose that this is not the only place where you are using this approach..

[JeremyCrookshank]

Tested all, seems to work well nice job. Will probably need a thorough test but also fixes my issue you tagged above. Your solution is much nicer than the one I hacked together meanwhile haha