search

sdcoffey / techan

Technical Analysis Library for Golang
https://godoc.org/github.com/sdcoffey/techan
MIT License
836 stars 142 forks source link

Proposal: Use shopspring/decimal instead of bespoke decimal interface #34

Open sdcoffey opened 3 years ago

sdcoffey commented 3 years ago

Shopspring's decimal is undoubtedly a better interface than my own big I propose we migrate away from big towards the better decimal impelmentation

tomarus commented 3 years ago

Hey, i've just ported this package to use shopspring/decimal but i run into some issues.

  1. The shopspring pkg does not support Sqrt() yet, but a PR is currently open and being worked on so this will probably be ready in a few weeks. See shopspring/decimal#130 I just copied the (apparently broken) Sqrt() from the PR and it seems to pass all tests lol.

  2. Shopspring doesn't support Inf. I've fixed this using the following but i have no idea if that's mathematically even the correct way to do this:

var plusInf = decimal.NewFromInt(math.MaxInt64)
var minInf = decimal.NewFromInt(-math.MaxInt64
  1. There is a HUGE increase in resources using shopspring/decimal, especially memory allocation. This is especially noticable in reecursive calculations, like MMA. I noticed this immediately when i had my own app ported. In fact it is also noticable by just running the tests.

This is the time for the tests to run on your own big package:

$ time go test -cover
PASS
coverage: 98.5% of statements
ok      github.com/sdcoffey/techan  0.040s

real    0m0.978s
user    0m2.532s
sys 0m0.549s

But when converted to shopspring/decimal it's more like this:

$ time go test -cover
PASS
coverage: 98.0% of statements
ok      github.com/tomarus/techan   20.757s

real    0m21.720s
user    0m26.858s
sys 0m1.660s

So that's like 20 times slower and 20 times the memory usage (don't have real stats from that).

I tried messing around with the MMA calculations a bit but it probably needs to be rewritten completely, somehow without recursion or so, But i'm really not knowledgeable about this.

Also i'm not sure where to go from here. I really like shopspring/decimal but not at this cost. Just wanted to let you know above findings. And thanks a lot for this package, it's really useful!

Here's my port if anyone wants to check it out: https://github.com/tomarus/techan/tree/decimal

sdcoffey commented 3 years ago

Wow, this is super insightful @tomarus! Thanks for the in-depth review. I think this would be part of a larger overhaul as a move toward v1, and definitely not something we'd do on a whim. Will definitely keep this in mind going forward