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

Feature request: Add support for rust_decimal. #55

Open
lukesneeringer opened this issue Dec 1, 2021 · 7 comments · May be fixed by #59
Open

Feature request: Add support for rust_decimal. #55

lukesneeringer opened this issue Dec 1, 2021 · 7 comments · May be fixed by #59

Comments

@lukesneeringer
Copy link

It would be nice to be able to use rust_decimal.Decimal and not lose the precision of f64. I would be willing in principle to contribute this if you think you would accept it.

@greyblake
Copy link
Owner

@lukesneeringer Hi.
This question was already raised in #15 .
I tend to say that we do not need it and close the issue, but I'd like to hear from you what is the necessity to have Decimal support?

@lukesneeringer
Copy link
Author

My reason is basically the same as the one in #15: I am trying to avoid the use of floats; every number in the project is an integer or a decimal.

I agree that for many of the outputs of ta, precision is less important, but it is not unimportant. Also, a lot of the folks writing against the API I am writing are less technical and I do not want to give them one sub-integer type consistently. This is also important because decimals support all the arithmetic operators with each other, but not with f64s -- meaning if I give my users a f64 in some places and a decimal in others, comparing them or manipulating them is more difficult than it ought to be.

I also expect this can be done in a reasonably generic way to not add very much code duplication. rust_decimal::Decimal supports all the arithmetic traits, after all.

@greyblake
Copy link
Owner

Decimals important for storing money and making operations with money, I do agree. But this does not apply to technical analysis.
I would argue that precision of f64 is more than needed for TA, and it's not enough, then you're probably fooling yourself, thinking that being super precise gives an advantage to your trading strategy.
As Warren Buffet once said: "It is better to be approximately right than precisely wrong."

meaning if I give my users a f64 in some places and a decimal in others, comparing them or manipulating them is more difficult than it ought to be.

You can implement a wrapper around the existing indactors, that would convert f64 -> Decimal. But generally I would suggest.. just convert the values where you want to do comparison. Moving averages, is probably the only case when you actually want to compare prices with indicator values.

@lukesneeringer
Copy link
Author

then you're probably fooling yourself, thinking that being super precise gives an advantage to your trading strategy.

It is not so much that I think the precision of decimals is critical in this case. I am much more interested in API simplicity. I want my folks to work with decimals all the time rather than having to take on converting between decimals and floats.

@lukesneeringer
Copy link
Author

@greyblake Following up on this, because it's still something I am hoping for. I'm willing to implement it myself, but I don't want to do it if you're going to reject the PR. :-)

I'd do it behind a feature gate, so it won't affect anyone who doesn't want it.

@greyblake
Copy link
Owner

@lukesneeringer Hi, thanks for reaching me.
I still hold the same opinion as before (decimal is MUST for prices, money amounts in bookkeeping, but not for TA indicators).

Unfortunately I don't have much spare time at the moment to dedicate to ta.
I am not sure what to suggest. Maybe you can do it a for 2-3 indicators and ask me to take a look?

@lukesneeringer
Copy link
Author

That's fair. I think I know how this should be done that would work well. I'll send you a PR.

@lukesneeringer lukesneeringer linked a pull request May 31, 2022 that will close this issue
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 a pull request may close this issue.

2 participants