Conversation
|
Oh I should add I feel a bit weird about writing any tests here. The rounding we had was acceptable before this change and adding a ruby/spec which specifies exact rounding would be the wrong thing to do. I still feel this reduces our friction from a minor incompatibility. |
|
This change happens to break a test which fell out of: https://bugs.ruby-lang.org/issues/10135. The funny bit is they rejected the script as invalid due to the fact that float cannot round to base 2. Obviously they made it work but looking at source I think maybe it is the use of long long changing the rounding? I am still somewhat ok with this PR because we round better in 2 libraries where I noticed us rounding differently. I did not check to see but I suspect we do a tiny bit better with one divide instead of two divides because there seems to be an extra chance at hitting a non-base-2 value. Although my motivation was never to 100% match MRI as much as reduce reporting of improper rounding from people who notice the discrepancy. |
|
that test exclude looks suspicious ... otherwise looking good |
|
@kares yeah so there is still some rounding issue where we can get different results. MRI even has two different compile modes for to_f although one uses rational so that one is fine. The other was a change in the last few years to use long long and somehow it rounds better than it did in the past? At the time that issue was opened though it rounded like we round with this patch, so that difference was small enough they rejected the issue. My take on that test is it will not round exactly back to original number because it is not base-2 and it just happens to not round our way. With our non-long path we do two divisions so we increase our chances of our rounds not matching MRI. This new code path just removes one addition so we seem match a in more cases... |
headius
left a comment
headius
left a comment
There was a problem hiding this comment.
This seems fine to me but I'm bummed about the exclude also.
|
I am not going to land this right before 9.2.9.0 but I also want to do a little more testing to see how much the errors from new rounding here shift around. |
|
Punting to 9.3.1 to evaluate the test regression before merging. |
This logic makes sure the time can fit into a single long. If so we can do math operations which round in the same way as MRI. The limit appears to be some time April 2262. By using this logic I fixed 3 error in activesupport. I also fixed another difference I was observing in .to_f in Oj. Note: I am not saying what we were doing was wrong. The alternate math we did made an acceptably close value, but just making this change will reduce any noise we get from users complaining about the lack of alignment with MRI. The two main methods affected here are Time#- and Time.to_f.
This fixes the excluded test from #5729.
|
I rebased this on latest master and pushed some changes to fix the regression. We shall see how it lands in CI. @enebo I'm not sure what activesupport tests this fixed, so you may need to confirm they are still fixed. |
3 participants
This logic makes sure the time can fit into a single long. If so
we can do math operations which round in the same way as MRI. The
limit appears to be some time April 2262.
By using this logic I fixed 3 error in activesupport. I also fixed
another difference I was observing in .to_f in Oj.
Note: I am not saying what we were doing was wrong. The alternate
math we did made an acceptably close value, but just making this
change will reduce any noise we get from users complaining about
the lack of alignment with MRI.
The two main methods affected here are Time#- and Time.to_f.