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

modifications for round #16608

Merged
merged 3 commits into from
Apr 23, 2019
Merged

modifications for round #16608

merged 3 commits into from
Apr 23, 2019

Conversation

smichr
Copy link
Member

@smichr smichr commented Apr 8, 2019

References to other Issues or PRs

closes #12056

Brief description of what is fixed or changed

  • The round method has been modified to round to even by default
    in keeping with Python 3 semantics

  • The round method no longer casts args to float so precision
    is no longer lost.

  • To get Python 3 behavior in Python 2, import round from sympy.core.compatibility

Other comments

Python and SymPy use different methods of rounding so rounding to the right of the decimal is WYSIWIG in SymPy but not in Python:

>>> round(12.345,2)
12.35
>>> S(12.345).round(2)
12.34

This is because Python uses the full binary representation while SymPy rounds an integer based on the precision of the number, e.g. if a number n is stored as 0.3499999 with 2 digits of precision, SymPy would use 35 for the rounding since this is the closest integer to n*10**2.

cf discussion here

Release Notes

  • core
    • round added to compatibility
    • under Python 3, Number.round == round(Number)
    • Number.round rounds to even on tie and does so in cases that Python misses, e.g. round(12.345,2) -> 12.35 in Python but 12.34 in SymPy whereas round(4.5) -> 4 in both.
    • Integer input is now returned as Integer

@sympy-bot
Copy link

sympy-bot commented Apr 8, 2019

Hi, I am the SymPy bot (v147). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • core
    • round added to compatibility (#16608 by @smichr)

    • under Python 3, Number.round == round(Number) (#16608 by @smichr)

    • Number.round rounds to even on tie and does so in cases that Python misses, e.g. round(12.345,2) -> 12.35 in Python but 12.34 in SymPy whereas round(4.5) -> 4 in both. (#16608 by @smichr)

    • Integer input is now returned as Integer (#16608 by @smichr)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.5.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->

closes #12056

#### Brief description of what is fixed or changed

* The round method has been modified to round to even by default
in keeping with Python 3 semantics

* The round method no longer casts args to float so precision
is no longer lost.

* To get Python 3 behavior in Python 2, import round from sympy.core.compatibility


#### Other comments

Python and SymPy use different methods of rounding so rounding to the right of the decimal is WYSIWIG in SymPy but not in Python:
```python
>>> round(12.345,2)
12.35
>>> S(12.345).round(2)
12.34
```
This is because Python uses the full binary representation while SymPy rounds an integer based on the precision of the number, e.g. if a number `n` is stored as 0.3499999 with 2 digits of precision, SymPy would use 35 for the rounding since this is the closest integer to `n*10**2`.

cf [discussion here](https://groups.google.com/forum/#!topic/sympy/CgkxNTT85aI)

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
- core
  - round added to compatibility
  - under Python 3, Number.round == round(Number)
  - Number.round rounds to even on tie and does so in cases that Python misses, e.g. round(12.345,2) -> 12.35 in Python but 12.34 in SymPy whereas round(4.5) -> 4 in both.
  - Integer input is now returned as Integer

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@smichr
Copy link
Member Author

smichr commented Apr 8, 2019

@asmeurer, do you want this in 1.4 or should it wait?

@asmeurer
Copy link
Member

asmeurer commented Apr 8, 2019

It's too late for 1.4 unless it is an absolutely critical bug fix. I plan to do the final release tomorrow or Wednesday.

@smichr
Copy link
Member Author

smichr commented Apr 9, 2019

absolutely critical bug fix

Nope -- just creating a compatibilty version of round and changing default rounding semantics

@Sc0rpi0n101
Copy link
Member

Some of the doctests are failing
for eg, in pi.round()
it's Expected: 3
Got: 3. not 3.0 or 3
It's with the single number results only. All others are like 2.0I or 6.03.0*I(still failing as int was expected)
is this a bug with Travis?

@smichr
Copy link
Member Author

smichr commented Apr 9, 2019

Some of the doctests are failing

Thanks -- updated accordingly:

sympy\core\expr.py[33] .................................                   [OK]

@smichr
Copy link
Member Author

smichr commented Apr 9, 2019

It would be good to have another set of eyes check the changes.

Does how work as a rounding method keyword? An alternative would be rnd but that looks like "random" to me...but that is the variables used in evalf.py

sympy/core/expr.py Outdated Show resolved Hide resolved
sympy/core/expr.py Outdated Show resolved Hide resolved
@asmeurer
Copy link
Member

asmeurer commented Apr 9, 2019

Some tests that round(Float) matches round(float) in Python 3 would be good.

@smichr smichr force-pushed the round branch 2 times, most recently from 5841b0b to 28c4dac Compare April 9, 2019 23:56
@smichr smichr closed this Apr 10, 2019
@smichr smichr reopened this Apr 10, 2019
@smichr smichr force-pushed the round branch 2 times, most recently from 514afbb to 4c99f7a Compare April 10, 2019 04:10
@smichr
Copy link
Member Author

smichr commented Apr 10, 2019

I'm not sure what to do about rounding Integers. What should the following give and why?

>>> round(123)  # matches Python single-argument rules
123
>>> [round(S(123), i) for i in (-1,0,1)]  # preserves precision information
[1.2e+2, 123., 123.0]

@Sc0rpi0n101
Copy link
Member

Shouldn't round(123,0) be 123, an int
instead of 123. , a float with 0 decimal places?

@smichr
Copy link
Member Author

smichr commented Apr 10, 2019

See OP for how SymPy and Python differ in rounding algorithms.

@asmeurer , comparisons of types are now tested.

@smichr
Copy link
Member Author

smichr commented Apr 10, 2019

Shouldn't round(123,0) be 123, an int

In Python 3, yes. This is now the result given by SymPy in this branch.

@smichr smichr closed this Apr 11, 2019
@smichr smichr reopened this Apr 11, 2019
@codecov
Copy link

codecov bot commented Apr 11, 2019

Codecov Report

Merging #16608 into master will increase coverage by 62.751%.
The diff coverage is 75%.

@@              Coverage Diff               @@
##            master    #16608        +/-   ##
==============================================
+ Coverage   11.001%   73.753%   +62.751%     
==============================================
  Files          619       619                
  Lines       158846    158982       +136     
  Branches     37254     37292        +38     
==============================================
+ Hits         17476    117254     +99778     
+ Misses      140684     36301    -104383     
- Partials       686      5427      +4741

@smichr smichr closed this Apr 11, 2019

assert (Float(.3, 3) + 2*pi).round() == 7
assert (Float(.3, 3) + 2*pi*100).round() == 629
assert (Float(.03, 3) + 2*pi/100).round(5) == 0.09283
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the output is limited by the precision of 3

@smichr smichr force-pushed the round branch 3 times, most recently from 5c315d0 to 35bfdd5 Compare April 13, 2019 02:58
@smichr smichr added PR: author's turn The PR has been reviewed and the author needs to submit more changes. and removed PR: sympy's turn labels Apr 13, 2019
@smichr smichr force-pushed the round branch 2 times, most recently from 1dfc48a to 7a00309 Compare April 14, 2019 11:28
@smichr smichr closed this Apr 14, 2019
@smichr smichr reopened this Apr 17, 2019
@smichr smichr removed the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Apr 17, 2019
- don't recast arg to float
- Integer in Integer out
- single arg call (n=None) gives Integer out
@smichr
Copy link
Member Author

smichr commented Apr 18, 2019

I feel like the test that were in place and those that I have added are a good test of this algorithm. I am also happy that we have round-to-even rounding working for Floats. The simplified code here belies the difficulty of getting this right. If anyone else wants to give this a check that would be nice, otherwise I will commit this post-Easter.

@codecov
Copy link

codecov bot commented Apr 18, 2019

Codecov Report

Merging #16608 into master will decrease coverage by 0.014%.
The diff coverage is 83.333%.

@@              Coverage Diff              @@
##            master    #16608       +/-   ##
=============================================
- Coverage   73.841%   73.826%   -0.015%     
=============================================
  Files          619       619               
  Lines       159332    159354       +22     
  Branches     37390     37396        +6     
=============================================
- Hits        117653    117646        -7     
- Misses       36249     36275       +26     
- Partials      5430      5433        +3

@oscarbenjamin
Copy link
Collaborator

+1

Should a new issue be opened with the dropping Python 2 label noting to follow up on this later?

@smichr
Copy link
Member Author

smichr commented Apr 19, 2019

cf #16686

@cbm755
Copy link
Contributor

cbm755 commented Apr 20, 2019

I'd like to (eventually) see Float rounding done by mpmath (mpmath/mpmath#455)

Then we could worry separately about how Rationals, irrational Numbers, etc are dealt with, without the code for Floats being interspersed. But that could wait until Python 2 is dropped, so I'm ok in principle with some of these changes.


But the round-to-even bit is suspicious for Floats. I don't believe the "augment precision, add 5 to previously least significant digit" logic.

Here is a float, converted to a Float, that is less than 575/1000 but none-the-less rounds (incorrectly) to 0.58:

>>> a = Float(0.575 - 6e-16)
>>> a < Rational(575, 1000)
True
>>> round(a, 2)
0.58

@smichr smichr closed this Apr 22, 2019
@smichr
Copy link
Member Author

smichr commented Apr 22, 2019

@cbm77, please see inline notes to the algorithm which is now recast more explicitly in terms of known digits as an integer and the rounding digit in the 10ths place.

@smichr smichr merged commit b176f6a into sympy:master Apr 23, 2019
@smichr smichr deleted the round branch April 23, 2019 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

round for Float loses precision
6 participants