Fix float round to zero

[fish-face]

Description of the issue/feature this PR addresses:

precision_rounding = 0 should mean "as much precision as possible", and not cause
unexpected incorrect calculations. In particular, rounding with a precision of 0 should not
change the input. The same is true if a huge number of digits of precision are requested
(over 323). There is also no situation where it makes sense to say that the literal value
0 is not zero, regardless of how much precision is requested.

This fixes float_round and float_is_zero for both of these issues.

An alternative would be to raise a ValueError if given precision_rounding of zero, or a
value of precision_digits high enough to have the same effect. I see no advantage in
that unless it is suspected that many users of these functions are already passing in a
value of zero, which hopefully is not the case, since they'll be getting wrong outputs!

Current behavior before PR:

Passing 0 results in rounding everything to zero and declaring that 0 is not zero

Desired behavior after PR is merged:

Passing 0 will result in no rounding, and correctly reporting that 0 is zero.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

[odony]

Woa, that's a very opinionated change of semantics of our API.

The doc is pretty clear that precision_rounding represents the minimum non-zero value at the desired precision[1]. Our rounding mechanisms are not intended to support no-op rounding, why would you try to use them for that?

It's not clear to me that we want to give callers the illusion that there is such a thing as "infinite precision" with floats, or that they can somehow decide to ignore rounding problems and still use float_round.
If you want Float fields with no rounding just use fields.Float(digits=0), and there will be no call to float_round.

And then of course we have to keep in mind that there are many users in production on 12.0, and they could rely (even unknowingly) on the current behavior. This can't be considered a bugfix, so it would have to target master, and include relevant changes to the doc and tests, I guess.

[1] https://github.com/odoo/odoo/pull/37204/files#diff-1853706ffae19f724b24c22c740de41bL47

[odony]

Perhaps you could illustrate your change with a supporting use case? That would help to understand the rationale :-)

[fish-face]

Thanks for looking at this.

The doc is pretty clear that precision_rounding represents the minimum non-zero value at the desired precision

If this is really the intention, then these functions should all raise an exception if passed zero, or more digits than can be represented by a non-zero precision. Silently saying that zero is not zero is not correct, no matter what the docstring says. The docstring also does not cover the possibility that an end-user provides, say, 1000 digits to mean "more digits than I care about". You could "solve" this by also adding a range check on the number of digits, but this makes still less sense mathematically.

It's not clear to me that we want to give callers the illusion that there is such a thing as "infinite precision" with floats

Float imprecision is understood by every programmer; this is not a reason to to make functions less useful. Rounding functions should behave similarly to the round builtin; round(3.1, 1000) does not return 0. In other words, rounding with "more precision than can be given" is best represented by a no-op, could conceivably be an error, but can never mean "return nonsense".

This can't be considered a bugfix

Either these functions should be behaving mathematically correctly or they should be raising errors. This is a bugfix, but I can rebase against master if preferred since it's of course not backwards incompatible.

supporting use case?

This came up for us when a unit of measure on a model was unset and we were taking the rounding value from the UoM; when unset this gives you 0. Of course there are other ways of working around this issue (as we have already implemented) but they all involve making calling code more complex for no good reason - you are asking that everything that calls float util functions with precision determined (directly or otherwise) by user input make a special check for something that the underlying function could handle correctly. As was already pointed out, users of this code may already be calling it without making this check.

Summary:

1. No matter what the docstring says, the most sensible behaviour is for rounding with "too much" precision to be a no-op
2. No matter what the docstring says, float_is_zero(0, ...) cannot return False no matter what ... is.
3. If the docstring behaviour is sacrosanct, an error must be raised.
4. This can be encountered "in the wild" whenever the rounding (precision or digits) depends (even indirectly) on user input.
5. I can rebase this against master as needed, but this is a bug.

[C3POdoo]

Dear @fish-face,

Thank you for your contribution but the version 12.0 is no longer supported.
We only support the last 3 stable versions so no longer accepts patches into this branch.

We apology if we could not look at your request in time.
If the contribution still makes sense for the upper version, please let us know and do not hesitate to recreate one for the recent versions. We will try to check it as soon as possible.

This is an automated message.