Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestions for possible improvements #19

Open
eugene-kamenev opened this issue Jul 29, 2022 · 9 comments
Open

Suggestions for possible improvements #19

eugene-kamenev opened this issue Jul 29, 2022 · 9 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@eugene-kamenev
Copy link

Hi Franklin,

Thank you for this collection of indicators!
Here are some suggestions which you may decide to apply:

  1. Design each indicator as a simple function
    This kind of design should be considered because of multiple reasons:
  • Imagine having an infinite stream of data, this will allow to save memory and maybe even increase performance.
  • Having simple functions allows composition
  • Having simple functions allows easy testing and rewriting to different language.
    Please let me give you quick example of simple SMA in Java that covers all three points above:
public static class SMA implements Function<BigDecimal, BigDecimal> { 
   private final var buffer = new LinkedList<>()
   private final int length
   private var sum = BigDecimal.ZERO

   SMA(int length) {
     this.length = length
   }

   @Override
   BigDecimal apply(BigDecimal currentValue) {
        if (buffer.size() == length) {
            sum -= buffer.removeFirst()
         }
        sum += currentValue
        buffer.addLast(currentValue)
        // at the end calculated SMA should be rounded to SOURCE scale
        return roundValue(sum / buffer.size(), currentValue)
  }
}

So it is basically a function, that accepts decimal value and produces new one. It has some minimal internal state like a self buffer etc. Usage is quite simple, it can be applied now to any datasource, streaming or not. RSI from SMA will look like this:

var datasource = candlesticks.map(c -> c.close)
     .map(new SMA(3).then(new RSI(14)))

I think you may improve your code in this way if you agree.
2. Rounding of values is a problem now. You used 4 decimals by default in any calculation, but I think this is not correct.
Whenever you know the scale you should use it. For example any moving average should use scale of original sourced value. RSI should/may use just 2 decimals. SMA from RSI should have the same 2 decimals for calculated value.
3. I tried to use some cryptocurrency candlestick data and many indicators failed to generate any useful data, I even couldnt find simple signal for RSI with StrongSell/StrongBuy values, which is weird.

The most important part is point with number 1, I do not have much experience with C# to help you with that. Please let me know what do you think.

@ooples
Copy link
Owner

ooples commented Jul 29, 2022

@eugene-kamenev Thanks for the great feedback! I do understand the use case for this functionality as far as moving to functions goes but my immediate thoughts were how would this work with streaming for example. If you were streaming stock data to a list then unless I'm mistaken, you would have issues with your example with not being able to access previous values and if you were streaming then you would have to calculate from the beginning of the list each time due to formulas like sma where you need to get the last X values for example. And of course there is an issue with the super complicated formulas that I have written like Ehlers stuff or some that are recursive in nature so I'm not totally sure how these use cases would look like. That being said, I do like the idea and I'm very open to increasing performance and decreasing memory usage.

I think you are totally correct about the rounding issue and clearly I will need to fix the rounding issues so that it is adaptive based on the source data that it is calculating based on. I'm open to ideas for how to do this concept but I will come up with some ideas for how to better do this in the meantime.

@eugene-kamenev
Copy link
Author

@ooples I would really like to help but, I just started learning C# code with your project. I am mainly Java developer, but I was able to rewrite some of your code to review the data it generates from most of indicators. I have some reflection based C# code to get all indicators list and generate the data, please let me know if it can help.
Now going back to functions approach. As I said above function can have a state, this means you can cache values, have some buffers defined, to collect history values you need, to produce next value. If you will look at SMA example above, it has a buffer of previous values to calculate current value. It is up to stream consumer whenever it is decided to cache values or do something with data. Function only results in a value, and we only expect new values of the source to come each time. All you have to do is to remove all loops you have to calculate indicators, extract each one into a function. Combine functions when needed. Then you should be able to subscribe to some price datastream and produce indicator values in realtime.
In Java we have a BigDecimal class which has all information about scale/precision of the value. So I basically round the value to original scale. Something similar should be available in C#. I am not sure if you should pay attention to each operation or only round resulting value.

@ooples
Copy link
Owner

ooples commented Jul 29, 2022

@eugene-kamenev Would it be too much to ask you to write some test code in Java with a slightly more complicated calculation like Weighted Moving Average for example and compare it to a java version of what I'm already doing to see if this will improve memory usage and speed? I'm open to making these changes in a C# version but would need to see proof that it would improve the performance enough to justify the many hours it would take me to make these changes. As far as the rounding goes, I'm going to fix this issue in the next version once I figure out a good way to handle it.

@eugene-kamenev
Copy link
Author

@ooples Just a quick rewrite of your version, hopefully logic is the same.
It still uses a loop for a buffer:

public static class WeightedMovingAverage implements Function<BigDecimal, BigDecimal> {
        private final LinkedList<Double> buffer = new LinkedList<>();
        private final int length;

        public WeightedMovingAverage(int length) {
            this.length = length;
        }

        @Override
        public BigDecimal apply(BigDecimal v) {
            if (buffer.size() == length) {
                buffer.removeLast();
            }
            buffer.addFirst(v.doubleValue());
            double sum = 0;
            double weightedSum = 0;
            int j = 0;
            for (var bufferValue : buffer) {
                double weight = length - j;
                sum += bufferValue * weight;
                weightedSum += weight;
                j++;
            }
            BigDecimal wma = weightedSum != 0 ? new BigDecimal(sum / weightedSum) : BigDecimal.ZERO;
            return roundValue(wma, v.scale(), RoundingMode.FLOOR);
        }
    }

Thank you @ooples

@ooples
Copy link
Owner

ooples commented Jul 29, 2022

@eugene-kamenev The only difference I can see is that you are ordering by dates in descending order and I'm ordering by dates in ascending order. Let me know when you have a java version of how I do my code so we can see which version uses less resources and runs faster when you can do a side by side

@ooples ooples added enhancement New feature or request help wanted Extra attention is needed labels Mar 27, 2023
@1nfected
Copy link

+1 on the suggestion to decouple all the indicators from the StockData class and design them as stand-alone functions.

This way, they can then be used with any arbitary series of data.

Right now, it feels a little un-intuitive to do everything through the StockData class.

(I've just started exploring this library, so please excuse me (and correct me) if I've made some wrong assumptions here.)

@ooples
Copy link
Owner

ooples commented Aug 2, 2023

@1nfected The whole reason for the StockData class is because there are many indicators that use different inputs such as some indicators that use just the close prices and some that use just the high and low prices. This class allows us to follow best design practices and reuse the same object for all indicator calculations and making them interchangeable

@1nfected
Copy link

1nfected commented Aug 4, 2023

@ooples, for certain indicators, I can understand that require specific data points, but if we look at basic indicators such moving averages, there could be a need to calculate MA of literally any data point, for e.g. "Average Price", "Total Traded Value" etc. And maybe even MA of RSI, who knows!

So, I think it would make sense to expose a way to do this with any arbitrary user supplied data points.

@ooples
Copy link
Owner

ooples commented Aug 4, 2023

@1nfected I understand what you are saying and I have contingency plans using the StockData class to take care of all scenarios you have listed which is why I'm using it for this project. My library can calculate indicators of indicators of indicators using any combo of inputs values and output values using my StockData class. You can use my code to do Ma of Rsi or whatever combo you want

@ooples ooples self-assigned this Oct 21, 2023
@ooples ooples added the good first issue Good for newcomers label Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants