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

Prefer Decimal over float in utils #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mhluongo
Copy link
Contributor

In financial applications, it's important to avoid floating-precision math. Instead, stick with integers (and either don't divide, or round conscientiously) or use a good arbitrary-precision math library.

In Python, that library is decimal. It supports all the operators you'd expect, properly "coerces" integers into Decimals, and throws an exception if fixed-precision and floating-point math mix.

I didn't go through the whole code base- I just fixed a couple things in utils I wanted to use. The tests appear to be unaffected, though I'm not sure they have the coverage for a change like this, so use caution when merging.

@mflaxman
Copy link

Thanks @mhluongo and good call, this is better. Would you mind adding test coverage for these changes? I'm requiring test coverage of all new PRs.

I realize it's a bit unfair that you're improving something with no test coverage and it requires test coverage in order to be merged. If it's a pain, I'm happy to write the tests for you.

Thanks again @mhluongo!

@mhluongo
Copy link
Contributor Author

I'll see if I have some time in the next couple days.

@mflaxman
Copy link

Thanks @mhluongo !

@eric20141111
Copy link

still not merged yet ......

@quentinlesceller
Copy link
Contributor

I think that would still require some test coverage to be sure this is not breaking anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants