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

float_round does the opposite of rounding when precision_digits is 5 or certain other values, completely breaking fields.Float type cached values #7024

Open
leio opened this issue Jun 10, 2015 · 26 comments
Labels
Confirmed the bug was confirmed by testers

Comments

@leio
Copy link

leio commented Jun 10, 2015

When a digits precision is 5, everything breaks down. In addition to many of the places that fail to call float helper functions (to clean up after lack of Decimal usage) after doing math, the function float_round, meant exactly for that, goes wrong and does the opposite of rounding.

Due to that, if decimal accuracy of Product Unit of Measure to 5 the clean values found in the database (e.g 1000.00000) will be put to ORM cache non-rounded and no otherwise math involved comparisons go wrong.

E.g added debug code to fields.Float.convert_to_cache method:
Adding value 1000.0000000000000000000000000000000000000000 to cache as 1000.0000000000001136868377216160297393798828 (digits=(16, 5)) res: stock.move()

It is calling float_round(value, precision_digits=digits[1]), which in case of digits[1] == 5 is wrong:

(Pdb) from openerp.tools import float_round
(Pdb) "%5.40f" % float_round(1000.00000, precision_digits=3)
'1000.0000000000000000000000000000000000000000'
(Pdb) "%5.40f" % float_round(1000.00000, precision_digits=4)
'1000.0000000000000000000000000000000000000000'
(Pdb) "%5.40f" % float_round(1000.00000, precision_digits=5)
'1000.0000000000001136868377216160297393798828'
(Pdb) "%5.40f" % float_round(1000.00000, precision_digits=6)
'1000.0000000000000000000000000000000000000000'
(Pdb) "%5.40f" % float_round(1000.00000, precision_digits=7)
'1000.0000000000000000000000000000000000000000'
(Pdb) "%5.40f" % float_round(1000.00000, precision_digits=8)
'1000.0000000000000000000000000000000000000000'
(Pdb) "%5.40f" % float_round(1000.00000, precision_digits=9)
'1000.0000000000001136868377216160297393798828'

And so not even carefully using these helper functions work, because actual stored values are put in the ORM cache non-rounded unexpectedly, so now things like database backed move.product_uom_qty are non-rounded, and can't fix that at all on the client-side, nor on server-side as float_round calls would just keep it broken still.

As a counter-example, decimal module handles this fine when presented with this broken value:

(Pdb) src = Decimal(float_round(1000.00000, precision_digits=5))
(Pdb) src
Decimal('1000.0000000000001136868377216160297393798828125')
(Pdb) src.quantize(Decimal('0.00001'))
Decimal('1000.00000')
(Pdb) "%5.40f" % float(src.quantize(Decimal('0.00001')))
'1000.0000000000000000000000000000000000000000'

@gurneyalex
Copy link
Contributor

@mart-e your input on this issue is very welcome.

@mart-e
Copy link
Contributor

mart-e commented Jun 12, 2015

Hello,

The issue in this particular case is that 100000000 * 0.00001 produce a binary representation error.
This kind of debate is recurrent because whatever we do, we always find corner cases. For instance #4189 was an experiment to fix a corner case (not this one) but we didn't merge it because of potential side effects (don't rely on black magic of internal representation of float).
I doubt we will ever be able to write a float_round method covering 100% cases but we should rely on helpers (like float_compare or float_is_zero) to manipulate floats correctly.

What are you trying to achieve exactly ? Yes float_round(1000, 5) will produce an error but if you compare with float_compare, it will work.

>>> from openerp.tools.float_utils import *
>>> float_round(1000, precision_digits=5)
1000.0000000000001  # decimal imprecision
>>> float_compare(1000.0, float_round(1000, 5), 5)
0  # considered as equal

Depends of the scenario but the solution is usually to use float_compare or float_is_zero.

@leio
Copy link
Author

leio commented Jun 12, 2015

In this case I have a comparison on the UI side, more exactly (simplified for a stock.move treeview):
red:product_uom_qty > reserved_availability

Somehow reserved_availability is exactly 1000.0 while product_uom_qty is 1000.0000000000001136868377216160297393798828.

Either way, if this situation were to stay, we would have to start somehow roundtripping from javascript originating values to server for round_float too or something...

Why not just use Decimal, even if just to convert to it inside float_round, do the quantize and return back as float? Or does that have issues in some combination of values as well?
We all know what the real solution here would be...

@mart-e
Copy link
Contributor

mart-e commented Jun 25, 2015

I see, the issue is that we have no float_compare method client side (the evaluation for colors is done client side). Implementing one is easy (we already have float_is_zero) but integrating it with py.js and having a correct decimal precision is less straightforward.

@mart-e mart-e added the Confirmed the bug was confirmed by testers label Jun 25, 2015
@leio
Copy link
Author

leio commented Jun 26, 2015

well, all that is complicated on javascript side as well of course, as no decimal type used there either (with something like decimal.js I suppose)... Meanwhile if only it would actually round correctly for digits=5, it would avoid triggering the mismatches in most cases at least.
I would like to see float_round fixed as a priority, while what you say would be nice as well of course, but not very useful or nice until it just automatically works when >, <, >= and so on are used in javascript handled attributes (like color).
If I would be willing to sign the CLA at this point, I would just propose a patch that just converts float_round to go through python decimal.Decimal.quantize, test if that works with all those decimal_precisions and call it a day for now.

@leio
Copy link
Author

leio commented Jun 26, 2015

And as hinted before, to me handling monetary and other such accounting values via IEEE-754 float types is rather unheard of in this day and age and really unacceptable. The real long-term solution is full conversion to decimal.Decimal, about which I have not heard any plans to do.

@michaelkarrer81
Copy link

+1 for decimal.Decimal

@leio
Copy link
Author

leio commented Aug 23, 2017

This breaks (now?) even with computed fields that conditionally set the computed value to a database backed value (where it's correctly entered by user to e.g a 3.00000 value), e.g with simplified code (in reality the precision 5 comes from Product UoM decimal accuracy configuration and the computation isn't always this simple assignment):

class StockMove(models.Model):
    _inherit = 'stock.move'

    @api.multi
    @api.depends('product_uom_qty')
    def _get_effective_product_uom_qty(self):
        for rec in self:
            rec.effective_product_uom_qty = rec.product_uom_qty

    effective_product_uom_qty = fields.Float(compute=_get_effective_product_uom_qty, digits=(16, 5))

Results in shell debug session:

>>> m = env['stock.move'].browse(1)
>>> m.product_uom_qty
24.0
>>> m.effective_product_uom_qty
24.000000000000004

@pedrobaeza
Copy link
Collaborator

@mart-e has this been handled already?

@mart-e
Copy link
Contributor

mart-e commented Apr 5, 2018

@pedrobaeza no, so far, nothing has been developed to do so on the client side AFAIK

@pedrobaeza
Copy link
Collaborator

And do you think we should keep it opened or it should be something "Won't fix"?

@mart-e
Copy link
Contributor

mart-e commented Apr 5, 2018

@ged-odoo your call! Do you think we could have a float_compare at the client side level? e.g. a rule in the tree view red:product_uom_qty > reserved_availability

@leio
Copy link
Author

leio commented Apr 6, 2018

Please keep in mind that this issue was initially about server-side float_round not actually rounding properly with some precision values (e.g. 5). But if that is not fixed (it boggles my mind why not; it's trivial by going through decimal.Decimal, at least temporarily inside float_utils if not using it throughout the codebase), then this is causing issues for not being able to get same results on clientside, as can't replicate the failed rounding exactly from client-side.

@gurneyalex
Copy link
Contributor

please don't close this, this serves as a reminder that the issue exists, and it is a real issue. Otherwise you will get people opening similar issues.

@leio
Copy link
Author

leio commented Jul 31, 2018

I feel like I have wasted more time fighting with this than it would have taken to convert everything in Odoo over to Decimal myself...
Yes, this includes a lot of purely server code from user input. For example it's not possible to figure out if a user entered quantity for a 0.00001 rounding UoM is valid or not, to e.g. raise UserError if more precision is entered. RPC gives 13.00000, server wants 13.000000000000002....

@vedler
Copy link

vedler commented Nov 19, 2019

Still present, I'll give another associated example regarding prices.

We have a system where we have for example a 750 ml bottle of wine on sale in eCommerce and would like to show the liter price of the product as well.

For that, we divide the sale price by the bottle size (in liters) and round the price to "Product Price" precision of 2 decimal places.

Now if the product price happens to be 4.79, the result of the division is:
4.79 / 0.75 = 6.386(6)
represented by 6.386666666666667 when printing to logs.

Rounding this with float_round and with precision_digits = 2 (default "Product Price" precision), the rounding gives the result of 6.390000000000001. Rounding this resulting value the second time will not fix the incorrect float decimals if the digits is set to 2, 3, 4, 5, 8, 9, 10 or 13 digits.

Our customers have their other servers requesting for the product prices + the liter prices directly over API, so it's not just a problem of showing this value as a string in eCommerce.

Snippet used for testing:

import logging
_logger = logging.getLogger(__name__)
test_price = 4.79
qty_per = 0.750
_logger.info("First price: %s" % test_price)
_logger.info("Quantity : %s" % qty_per)
unrounded_res = test_price / qty_per
_logger.info("Unrounded result: %s" % unrounded_res)
rounded_res = float_round(unrounded_res, precision_digits=2)
_logger.info("Rounded result (float_round to 2 digits): %s" % rounded_res)

for i in range(15):
    _logger.info("Rounded result (float_round to %s digits): %s" % (i, float_round(rounded_res, precision_digits=i)))
    _logger.info("Rounded result (float_round to %s digits -> Py round): %s" % (i, round(float_round(rounded_res, precision_digits=i), i)))

My question is that if the rounding even fails with 2 to 4 digits, has it been proven not to affect accounting in this regard when sum operations are done on these values with high enough volumes of data? Are account moves safe from these rounding issues?

@mart-e
Copy link
Contributor

mart-e commented Nov 19, 2019

@vedler the problem in this issue is that you can not make proper float comparison in a view declaration (where the comparison is done client side, in javascript).
For the server side, you should use float_compare when doing float operations.

@vedler
Copy link

vedler commented Nov 19, 2019

@vedler the problem in this issue is that you can not make proper float comparison in a view declaration (where the comparison is done client side, in javascript).
For the server side, you should use float_compare when doing float operations.

Yes, comparisons are fine, but when using the intended rounding methods, I do not have a way to get rid of decimal imprecision if any external source needs to query for the values directly and instead have additionally use normal Python round() on top of it.

@mart-e
Copy link
Contributor

mart-e commented Nov 19, 2019

You can use also the float_repr method when you need to have a representation without the extra digitst

_logger.info("Rounded result (float_round to 2 digits): %s" % float_repr(unrounded_res, precision_digits=2)

But this is intended to be used at the last step of your processing, when you want to render the value. Use raw float numbers and float operations in your business code.

@vedler
Copy link

vedler commented Nov 19, 2019

I don't think this should be regarded as a minor issue. If one of the most commonly used utils in floating point operations throughout the code has these bugs, then in the end its hard to rely on it. In that customer's staging instance we have 2161 products they sell at their store imported (with prices, volumes etc.) out of which 138 products (6.38%) had this exact rounding issue present, where we relied on float_round. The number of products will be expected to reach 5-6+ digits once they go live.

The original development for our custom module was done under the impression that the util will not have such problems but it was the customer that reported the issue to us, even when we took the expected steps to mitigate this problem - in the end it's us who hold the responsibility. Again, to explain why it is an issue for us currently, is that the customer uses the Odoo API to request data directly (for their other systems) and does not do this through places, where such string conversions are even done. Yes, using the float_repr we can fix the displaying of these values in the frontend, but not the computations themselves without additionally adding the Python round() method which as I understand the float_round should be a replacement for. And yes, our own business code is not affected by this, rather its the problem in external sources requesting for our data.

If it is decided to not be fixed, I'd at least ask you to include a comment about this in the util itself and maybe even include the float_repr example.

@vedler
Copy link

vedler commented Nov 19, 2019

Another problem is using Monetary field itself, which applies this rounding automatically. If the value of the field is inputted as the correctly rounded value of 6.39 for example, the field's automatic rounding from the currency_id (currency.round(...)) "unrounds" it again to 6.390000000000001

@richard-willdooit
Copy link
Contributor

My question is that if the rounding even fails with 2 to 4 digits, has it been proven not to affect accounting in this regard when sum operations are done on these values with high enough volumes of data? Are account moves safe from these rounding issues?

I just had to help a client who has 3 months of data, and their trial balance differs debits to credits by 4 cents. Yes, insignificant, but to him, concerning, as how can they ever be different....

@vedler
Copy link

vedler commented Nov 28, 2019

What I'd also propose is to figure out how to lose the rounding errors at least when requesting data over XML-RPC for non-stored, computed fields. Precision problems don't seem to be an issue if the convert_to_column (which uses float_repr) has been used on the value at some point, but they are very common if they are requested directly from the cache.

In the end I guess it comes down to the question why slightly better performance is more valued over perfect precision for monetary values. What levels of performance loss are we talking about here? One can argue, that hitting much larger number of digits for float values is also plausible if you need another reason to consider decimal.Decimal over IEEE-754 floats in the future.

Side note: yes, I agree there isn't a need to change the system only for the smallest percent of possible customers, but I think demonstrating that the system would be capable of handling any plausible monetary values with perfect precision (handling companies of any size) is in my opinion a much bigger plus considering Odoo is at its core a financial application.

I'll throw in a simple-ish description of a system below, where these errors could plausibly be started to hit (where 16+ places of decimal precision are needed and things start falling apart).

The Example

A plausible scenario for hitting actual rounding error problems is modelling the following system:

  1. Petrol station company, tracking fuel by the gram (roughly ml)
  2. Vietnamese accounting (because of VND exchange rate) and VND currency rounding changed to 0.01
  3. Hitting (maybe very roughly about) 5-10% of market share in terms of turnover by plugging in arbitrary, but plausible numbers for product prices and line quantities
  4. Extrapolate data over 5 years worth (a likely starting amount of time that the company wants to preserve all accounting data for) to keep the quantities and prices plausible

What I tried out on Odoo 13 was:

  1. Create a product with 20.69 VND as price and grams as UoM (roughly the current Vietnamese petrol price)
  2. Create Sale Orders using kg as sale units and adding lines for every 10 days with roughly some plausible values (I used 5000 tonnes aka 5mil kg per line) and added 10 lines to every order (100 days of plausible turnover per SO)
  3. Duplicated the sale orders to 1700 days worth (17 orders * 100 days), correctly summing to a total of 17,586,500,000,000.00 VND untaxed at the bottom of the order list view (500t per day (line/10) -> 20.69 * 500000000g = 10345000000 VND per day which is roughly 405k € per day or 148mil € per year in untaxed turnover, which seem very plausible of larger Odoo customers).
  4. Generated and posted invoices
  5. Opened Profit and Loss report incorrectly rounding to 17,586,500,000,000.01 VND (14 digits and 2 decimal places)

Raise prices, quantities or time lapsed for the modelled data and it is possible that the rounding errors can be hit at more meaningful orders of magnitude or other currencies.

When I tried using a 13-digit quantity (6×10¹²), the roundings started breaking apart in the quantity field itself, untaxed, taxed and total fields. Additionally it was not possible to create an invoice from the Sale Order as it already reported a 5-cent difference from rounding problems, but this is to be expected from float problems already.

Using grams as the UoM for stock also means that the stock quantities of the products start getting close to the same rounding problems (500000000×1700 = 8,5×10¹¹ - 12+3 places) - I didn't check for any examples in Stock modules where similar issues could arise. The float util tests advertise tests for 17 significant decimal digits, but problems already rise at rounding values with 16 significant digits too due to that.

Sure you could argue that when you keep using your data only inside Odoo and use float_compare/float_is_zero everywhere, then everything should operate the same (except for already the 13 digit + 3 decimal places quantity on a sale order line, where only 12+3 digits is actually usable).

@etobella
Copy link
Contributor

etobella commented Jan 14, 2020

Well I made a small proposal on #43278 and this is really weird....
I made a test that checked 10000 random values checking both functions: and this was the result depending on the precision digits

  • 1 digit: Current function: About 30% of the numbers failed, My function: 0% failure
  • 2 digit: Current function: About 10% of the numbers failed, My function: 0% failure
  • 3 digit: Current function: About 14% of the numbers failed, My function: 0% failure
  • 4 digit: Current function: About 32% of the numbers failed, My function: 0% failure
  • 5 digit: Current function: About 55% of the numbers failed, My function: 91% failure
  • 6 digit: Current function: About 30% of the numbers failed, My function: 0% failure
  • 7 digit: Current function: About 30% of the numbers failed, My function: 0% failure
  • 8 digit: Current function: About 14% of the numbers failed, My function: 0% failure
  • 9 digit: Current function: About 42% of the numbers failed, My function: 0% failure
  • 10 digit: Current function: About 24% of the numbers failed, My function: 0% failure

The difference is really small, but we can see that there is issues always when trying to aggregate the data.

@voronind
Copy link

voronind commented Jul 2, 2021

Well I made a small proposal on #43278 and this is really weird....
I made a test that checked 10000 random values checking both functions: and this was the result depending on the precision digits

Can you share the test?

khah-odoo added a commit to odoo-dev/odoo that referenced this issue Apr 28, 2023
Steps to reproduce:
1. Make a sale order with a product priced at 24.99
2. Add UPS international shipping
3. Confirm the sale order, and attempt to validate the sale order

Gets an error from UPS:
`Invalid or missing product unit value for product number 1.
Valid length is 1 to 19 numeric and it can hold up to 6 decimal places`

The reason for this is that `float_round` adds a rounding error.
Basically:
`float_round(24.99, precision_digits=2)` => `24.990000000000002`

(See discussion at odoo#7024)

opw-3289042
@PlantBasedStudio
Copy link

I have a similar problem when selling an item in a cash point of sale, the amount_total value takes multi-digit values like 12.40000008 instead of 12.40
I'm in a two-digit currency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Confirmed the bug was confirmed by testers
Projects
None yet
Development

No branches or pull requests

10 participants