Everything

php

[RFC] [Discussion] Change the edge case of round()

php.internals

Saki Takamachi

2 years ago

Hi, internals

I would like to start the discussion for my RFC "Change the edge case of round()”.
https://wiki.php.net/rfc/change_the_edge_case_of_round <https://wiki.php.net/rfc/adding_bcround_bcfloor_bcceil_to_bcmath>

Regards.

Saki

Saki Takamachi

2 years ago

Sorry, I posted a completely unrelated URL.

Correct url:
https://wiki.php.net/rfc/change_the_edge_case_of_round

Regards.

Saki

Max Semenik

2 years ago

This sounds like a subtle breakage to a lot of code. Could you elaborate
the motivation for such change? One sentence doesn't seem enough. Also,
have you tried assessing how much existing code might be affected?

On Mon, Oct 23, 2023 at 2:19 PM Saki Takamachi <saki@sakiot.com> wrote:

> Sorry, I posted a completely unrelated URL.
>
> Correct url:
> https://wiki.php.net/rfc/change_the_edge_case_of_round
>
> Regards.
>
> Saki
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

--
Best regards,
Max Semenik

Saki Takamachi

2 years ago

Hi, Max

This was originally discussed in a pull request. If you're interested in the original discussion, check out this pull request.
https://github.com/php/php-src/pull/12268

Regards.

Saki

Saki Takamachi

2 years ago

Hi,

It's almost been two weeks since I opened this discussion. If there are no other opinions, I am thinking of starting a vote tomorrow or later.

Regards.

Saki

Claude Pache

2 years ago

> Le 5 nov. 2023 à 08:31, Saki Takamachi <saki@sakiot.com> a écrit :
> ﻿Hi,
>
> It's almost been two weeks since I opened this discussion. If there are no other opinions, I am thinking of starting a vote tomorrow or later.
>
> Regards.
>
> Saki

Hi Saki,

Reading the RFC, it is unclear for me what are the benefits of the change, and whether it is worth the subtle breaking of existing code that implicitly relies on the current behaviour. “FP is just FP” looks like an ideal position, and one that has a counteragument given in the RFC that introduced the current behaviour.

We could live with any behaviour of round(), and they are arguments on both sides. My issue at this point is not exactly what behaviour we have picked, but whether changing between the two is sufficiently motivated.

—Claude

Saki Takamachi

2 years ago

Hi Claude,

> Reading the RFC, it is unclear for me what are the benefits of the change, and whether it is worth the subtle breaking of existing code that implicitly relies on the current behaviour. “FP is just FP” looks like an ideal position, and one that has a counteragument given in the RFC that introduced the current behaviour.
>
> We could live with any behaviour of round(), and they are arguments on both sides. My issue at this point is not exactly what behaviour we have picked, but whether changing between the two is sufficiently motivated.

As for the motivation for the change, it's based on the idea that the current `round()` behavior is some kind of bug.
In other words, the behavior of HALF_UP `0.285` to become `0.29` is itself a bug.

To be honest, I'm completely neutral on this. I believe that this is a matter of will and what we want to do. As far as I know, Tim and Gina want change on this issue. I don't want to ignore such opinions and move forward with implementation, so I would like to hear a wider range of opinions.

I have created this RFC in order to determine "our intention" once again.

Regards.

Saki

Michael Wallner

2 years ago

On Mon, 6 Nov 2023 21:12:40 +0900, Saki Takamachi wrote:

Hi Saki!

> To be honest, I'm completely neutral on this. I believe that this is a
> matter of will and what we want to do. As far as I know, Tim and Gina
> want change on this issue. I don't want to ignore such opinions and move
> forward with implementation, so I would like to hear a wider range of
> opinions.
>
> I have created this RFC in order to determine "our intention" once
> again.

TBH, the RFC is pretty skinny for such a controversial proposal. I see
that there are discussions in bug reports and pull requests, but the RFC
should condense that information, to be useful. Chances are pretty low as
it stands now.

I didn't look into other languages, like done in another discussion, but I
looked into some of the most popular RDBMS options, like SQLite, MySQL and
Postgres, of which all three seem to give 0.29 for your example.

Mike

Saki Takamachi

2 years ago

Hi Mike,

I'm still new to RFC and your advice would be very helpful.

I have added as much of the rationale for the changes as I can think of to the RFC. I would be happy if you could see it again.

Thank you, regards.

Saki

Jakub Zelenka

2 years ago

Hi,

please do not top post - you might want to check these rules for PHP
mailing lists:
https://github.com/php/php-src/blob/master/docs/mailinglist-rules.md if you
haven't seen them already ;)

On Tue, Nov 7, 2023 at 4:07 PM Saki Takamachi <saki@sakiot.com> wrote:

> I have added as much of the rationale for the changes as I can think of to
> the RFC. I would be happy if you could see it again.
>
>

It would be good to also mention original reasoning for preferring the
decimal edge cases. It says:

> Of course, one may argue that pre-rounding is not necessary and that this

is simply the problem with FP arithmetics. This is true on the one hand,
but the introduction of the places parameter made it clear that round() is
to operate as if the numbers were stored as decimals. We can't revert that
and this seems to me to be the best solutions for FP numbers one can get

I think it's got a good point with the second parameter and it's something
that voters should consider. You should not probably say that's current
implementation is wrong. It just chose to prefer decimal format and address
the edge cases explicitly in a way that it makes more sense to users.
Obviously it didn't fix all issues but we can potentially continue in that
approach which is what you did in one of your PR's - it would be good to
probably also note that such implementation is more complex.

Regards

Jakub

Saki Takamachi

2 years ago

Hi Jakub,

> please do not top post - you might want to check these rules for PHP mailing lists: https://github.com/php/php-src/blob/master/docs/mailinglist-rules.md if you haven't seen them already ;)

I intended to post with all the quotes deleted, but it seems I was wrong.

> It would be good to also mention original reasoning for preferring the decimal edge cases. It says:
>
> > Of course, one may argue that pre-rounding is not necessary and that this is simply the problem with FP arithmetics. This is true on the one hand, but the introduction of the places parameter made it clear that round() is to operate as if the numbers were stored as decimals. We can't revert that and this seems to me to be the best solutions for FP numbers one can get
>
> I think it's got a good point with the second parameter and it's something that voters should consider. You should not probably say that's current implementation is wrong. It just chose to prefer decimal format and address the edge cases explicitly in a way that it makes more sense to users. Obviously it didn't fix all issues but we can potentially continue in that approach which is what you did in one of your PR's - it would be good to probably also note that such implementation is more complex.

Ok, I'll fix the RFC. thank you for the advice.

Regards.

Saki

Saki Takamachi

2 years ago

Hi Jakub,

I added and modified into the RFC. I would be happy if you could check it out.

Regards.

Saki

Jakub Zelenka

2 years ago

On Wed, Nov 8, 2023 at 9:40 AM Saki Takamachi <saki@sakiot.com> wrote:

> I added and modified into the RFC. I would be happy if you could check it
> out.
>

It looks much better.

It might make sense to also put some info what other languages / dbs do.
For example Python round(0.285, 2) result is 0.28 and it's got also decimal
parameter. If you can find other implementations that have got such
function, that would be great!

Cheers

Jakub

Tim Starling

2 years ago

On 8/11/23 03:06, Saki Takamachi wrote:

> I have added as much of the rationale for the changes as I can think of to the RFC. I would be happy if you could see it again.

Thanks for the detailed RFC and the PR.

I reviewed the RFC and the PR. I would vote in favour of this change.

I look forward to seeing a PR for the manual which will better explain
the limitations of decimal rounding of a binary floating point number.
The current manual entry for round() makes it sound as if it will
magically always work.

I think, most often, round() is done on the result of some
calculation, not on decimal literals. The previous implementation
seems to prioritize rounding of decimal literals over correct rounding
of the actual value.

-- Tim Starling

Saki Takamachi

2 years ago

Hi Tim,

> I think, most often, round() is done on the result of some calculation, not on decimal literals. The previous implementation seems to prioritize rounding of decimal literals over correct rounding of the actual value.

Yes, as Jakub pointed out, there was a lack of explanation regarding existing implementations. I would like to add it to the RFC.

Regards.

Saki

Jordan LeDoux

2 years ago

On Mon, Oct 23, 2023 at 4:14 AM Saki Takamachi <saki@sakiot.com> wrote:

> Hi, internals
>
> I would like to start the discussion for my RFC "Change the edge case of
> round()”.
> https://wiki.php.net/rfc/change_the_edge_case_of_round <
> https://wiki.php.net/rfc/adding_bcround_bcfloor_bcceil_to_bcmath>
>
> Regards.
>
> Saki
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>
>

I think this makes much more sense if PHP programmers had internal access
to the float type in PHP, but that's not the case. You cannot choose
between float representations, you cannot easily examine the mantissa bits
directly. In the absence of stuff like that, I think the argument to treat
PHP floats as if they were decimals for the purposes of rounding is much
stronger.

That said, this sort of 'intelligent' rounding certainly introduces a large
number of edge cases. Additionally, if PHP programmers expect decimal
rounding to "just work", then it feels like anywhere that it doesn't work
the way they think it should is a bug, which isn't necessarily true.

I'm unsure whether or not this change actually improves anything. Is it
more correct from a technical perspective? Absolutely. Does it provide a
benefit for PHP programmers? That I am much less certain about.

Jordan

Saki Takamachi

2 years ago

Hi Jordan,

> I think this makes much more sense if PHP programmers had internal access
> to the float type in PHP, but that's not the case. You cannot choose
> between float representations, you cannot easily examine the mantissa bits
> directly. In the absence of stuff like that, I think the argument to treat
> PHP floats as if they were decimals for the purposes of rounding is much
> stronger.
>
> That said, this sort of 'intelligent' rounding certainly introduces a large
> number of edge cases. Additionally, if PHP programmers expect decimal
> rounding to "just work", then it feels like anywhere that it doesn't work
> the way they think it should is a bug, which isn't necessarily true.

Yes, that's a valid concern. Therefore, I am also proposing to add a function similar to round to BCMath. In a sense, we can think that we are looking for the same functionality in round() that we should be looking for in BCMath.

> I'm unsure whether or not this change actually improves anything. Is it
> more correct from a technical perspective? Absolutely. Does it provide a
> benefit for PHP programmers? That I am much less certain about.

In terms of "not lying about the results", this RFC makes us more precise about dealing with FP. However, in terms of benefits for users, there may not be any significant benefits.

Regards.

Saki