Skip to content

Conversation

@sgflt
Copy link
Contributor

@sgflt sgflt commented Mar 28, 2024

Fixes #1139.

Performance of partial sums.

Changes proposed in this pull request:

  • longer barSeries benefits from reusage of precomputed partial sums

  • random access fallbacks to slowPath

  • added an entry with related ticket number(s) to the unreleased section of CHANGES.md

@sgflt
Copy link
Contributor Author

sgflt commented Mar 28, 2024

Test on real data:
Before:
ta4jexamples.backtesting.MovingAverageCrossOverRangeBacktest -- Back-tested 2145 strategies on 6150-bar series in 72520 ms

After:
ta4jexamples.backtesting.MovingAverageCrossOverRangeBacktest -- Back-tested 2145 strategies on 6150-bar series in 6222 ms

@TheCookieLab
Copy link
Member

TheCookieLab commented Mar 29, 2024

Should provide a lot of value as this is one of the most commonly used indicators. Does this work as expected on a moving bar series?

@sgflt
Copy link
Contributor Author

sgflt commented Apr 3, 2024

I will add edge case tests, to prove functionality.

@sgflt
Copy link
Contributor Author

sgflt commented Apr 3, 2024

Umm, what is expected behavior for moving series?

If I add following test:

    @Test
    public void usingBarCount3UsingClosePriceMovingSerie() {
        data.setMaximumBarCount(13);
        data.addBar(new MockBar(5., numFunction));

        Indicator<Num> indicator = getIndicator(new ClosePriceIndicator(data), 3);

        assertNumEquals((0d + 0d + 2d) / 1, indicator.getValue(data.getBeginIndex() + 0));
        assertNumEquals((0d + 2d + 3d) / 2, indicator.getValue(data.getBeginIndex() + 1));
        assertNumEquals((2d + 3d + 4d) / 3, indicator.getValue(data.getBeginIndex() + 2));
        assertNumEquals((3d + 4d + 3d) / 3, indicator.getValue(data.getBeginIndex() + 3));
    }

It fails on second assert (0d + 2d + 3d) / 2 even with previous implementation.

Expected :2.5
Actual   :2.3333333333333335

data.getBeginIndex() + 0 returns bar at index 0 because i - removedBars < 0;
data.getBeginIndex() + 1 returns also bar at index 0 because i - removedBars == 0;

It seems unstable bars are really unstable and should not be tested?

@sgflt
Copy link
Contributor Author

sgflt commented Apr 8, 2024

Discovered RunningTotalIndicator. I may use this indicator as partialSum and optimize RunningTotalIndicator.

@sgflt
Copy link
Contributor Author

sgflt commented Apr 14, 2024

And now I have discovered PreviousValueIndicator. I think I may combine them too.

@sgflt
Copy link
Contributor Author

sgflt commented Apr 17, 2024

Nope with indexes.

@TheCookieLab
Copy link
Member

Yes, you'll want to handle unstable indexes as unstable and assert accordingly.

@TheCookieLab
Copy link
Member

@sgflt please run the formatter (mvn -B clean license:format formatter:format test) and re-push

TheCookieLab
TheCookieLab previously approved these changes May 17, 2024
@TheCookieLab
Copy link
Member

@sgflt I recently cut the 0.16 release and started 0.17. Can you update your CHANGELOG entry accordingly? Thanks.

@sgflt
Copy link
Contributor Author

sgflt commented May 19, 2024

Done

@TheCookieLab TheCookieLab merged commit ad120c0 into ta4j:master May 19, 2024
@sgflt sgflt deleted the sma-partial-sum-cache branch May 20, 2024 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SMAIndicator inner cache

2 participants