Calc package Decimal values potentially incorrect #104

Closed
ThaSiouL opened this Issue Aug 22, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@ThaSiouL

Hey, I just looked over your new Calc package and nocticed that even simple decimal calculations (e.g. 3/10) can be off by a bit and show more than 25 decimal places in the unnormalized form. It seems like the cause is the float to decimal conversion on line 393. I added a 14 digit precision to the float in my version which lets the function works as expected.

@polyvertex

This comment has been minimized.

Show comment
Hide comment
@polyvertex

polyvertex Aug 23, 2016

Member

I assume you meant line 387 (i.e. self.ans = decimal.Decimal(self.ans))? Well, the root of the problem is not this conversion itself, more the fact that we end up with a float here. Plus the fact that dealing with the approximate representation of floating point numbers by forcing a rounding precision is usually something one would prefer to avoid. Whether it being hard-coded or configurable. There's a possible solution to investigate though that would require some modifications in the _retokenize method.

Member

polyvertex commented Aug 23, 2016

I assume you meant line 387 (i.e. self.ans = decimal.Decimal(self.ans))? Well, the root of the problem is not this conversion itself, more the fact that we end up with a float here. Plus the fact that dealing with the approximate representation of floating point numbers by forcing a rounding precision is usually something one would prefer to avoid. Whether it being hard-coded or configurable. There's a possible solution to investigate though that would require some modifications in the _retokenize method.

@polyvertex

This comment has been minimized.

Show comment
Hide comment
@polyvertex

polyvertex Aug 23, 2016

Member

I realize I haven't been clear enough regarding the idea of a predefined rounding precision. What I meant is that predefined rounding precision may or may not be what the user wishes depending on the type of calculus being made. Because it's a changing thing, the patch you applied to your fork doesn't fit well IMO because you force a predefined rounding precision without leaving any chance to the user to know what the more precise result could look like.

Anyway, the changes applied to the _retokenize method work perfectly (typically for the kind of examples you mentioned; operation implying integers but that may result in a float on output) but is not enough to cover every corner case. So the package has been added a rounding_precision setting that will be applied to only one of the final results so user has a chance to get a more precise version if needed. I believe this covers pretty much every case now. Hopefully. Will be available in 2.9.1.

Member

polyvertex commented Aug 23, 2016

I realize I haven't been clear enough regarding the idea of a predefined rounding precision. What I meant is that predefined rounding precision may or may not be what the user wishes depending on the type of calculus being made. Because it's a changing thing, the patch you applied to your fork doesn't fit well IMO because you force a predefined rounding precision without leaving any chance to the user to know what the more precise result could look like.

Anyway, the changes applied to the _retokenize method work perfectly (typically for the kind of examples you mentioned; operation implying integers but that may result in a float on output) but is not enough to cover every corner case. So the package has been added a rounding_precision setting that will be applied to only one of the final results so user has a chance to get a more precise version if needed. I believe this covers pretty much every case now. Hopefully. Will be available in 2.9.1.

@ThaSiouL

This comment has been minimized.

Show comment
Hide comment
@ThaSiouL

ThaSiouL Aug 23, 2016

I just did it as a quick fix so it's great if you found a better way to fix it.

I just did it as a quick fix so it's great if you found a better way to fix it.

@polyvertex

This comment has been minimized.

Show comment
Hide comment
@polyvertex

polyvertex Aug 23, 2016

Member

Should be OK in v2.9.1 (released).

Member

polyvertex commented Aug 23, 2016

Should be OK in v2.9.1 (released).

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