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

Changes behaviour of round to use default rounding mode #9344

Merged
merged 3 commits into from Dec 22, 2014

Conversation

simonbyrne
Copy link
Contributor

This makes the changes discussed in #8750.

@JeffBezanson
Copy link
Sponsor Member

+1

Maybe a NEWS item?

@simonbyrne
Copy link
Contributor Author

done.

@johnmyleswhite
Copy link
Member

+1

@StefanKarpinski
Copy link
Sponsor Member

This is an impressively thorough pull-request. I was under the impression that we had settled on the default rounding mode matching C's round function, no? (I don't really care that much, tbh.)

@@ -3,17 +3,21 @@ include("fenv_constants.jl")

export
RoundingMode, RoundNearest, RoundToZero, RoundUp, RoundDown, RoundFromZero,
RoundNearestTiesAway, RoundNearestTiesUp,
get_rounding, set_rounding, with_rounding

## rounding modes ##
immutable RoundingMode{T} end

const RoundNearest = RoundingMode{:TiesToEven}()
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can we synchronize the names with the symbols? I.e. RoundX = RoundingMode{:X}()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which do we want to change? The symbols match the terms used in the ieee standard. The names are probably simpler.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

It doesn't really matter to me as long as they match. I would tend to prefer the IEEE names though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently have:

const RoundNearest = RoundingMode{:TiesToEven}()
const RoundToZero = RoundingMode{:TowardZero}()
const RoundUp = RoundingMode{:TowardPositive}()
const RoundDown = RoundingMode{:TowardNegative}()
const RoundFromZero = RoundingMode{:AwayFromZero}() # mpfr only, not in IEEE754
const RoundNearestTiesAway = RoundingMode{:TiesToAway}() # C-style rounding
const RoundNearestTiesUp = RoundingMode{:TiesToPositive}() # Java-style rounding, not IEEE754

Option (a): copy IEEE names

const RoundTiesToEven = RoundingMode{:TiesToEven}()
const RoundTowardZero = RoundingMode{:TowardZero}()
const RoundTowardPositive = RoundingMode{:TowardPositive}()
const RoundTowardDown = RoundingMode{:TowardNegative}()
const RoundAwayFromZero = RoundingMode{:AwayFromZero}() # mpfr only, not in IEEE754
const RoundTiesToAway = RoundingMode{:TiesToAway}() # C-style rounding
const RoundTiesToPositive = RoundingMode{:TiesToPositive}() # Java-style rounding, not IEEE754

Option (b), use simpler names

const RoundNearest = RoundingMode{:Nearest}()
const RoundToZero = RoundingMode{:ToZero}()
const RoundUp = RoundingMode{:Up}()
const RoundDown = RoundingMode{:Down}()
const RoundFromZero = RoundingMode{:FromZero}() # mpfr only, not in IEEE754
const RoundNearestTiesAway = RoundingMode{:NearestTiesAway}() # C-style rounding
const RoundNearestTiesUp = RoundingMode{:NearestTiesUp}() # Java-style rounding, not IEEE754

Option (c): same as (b), except for the last two, we could copy Python which uses "half" instead of "ties":

const RoundHalfAway = RoundingMode{:HalfAway}() # C-style rounding
const RoundHalfUp = RoundingMode{:HalfUp}() # Java-style rounding, not IEEE754

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I wish the IEEE names were better. Are those really the names in the standard or just what people use? The simpler names are so much clearer, shorter and more memorable. I guess we can go with IEEE names if they're actually commonly used in other systems (C, Python, Matlab, etc). Otherwise I would use the nicer, shorter names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think anyone uses the IEEE names. Having thought about it more, I agree that the simpler names are better.

@simonbyrne
Copy link
Contributor Author

@StefanKarpinski the proposal was to match C rint, #8750 (comment)

JeffBezanson added a commit that referenced this pull request Dec 22, 2014
Changes behaviour of round to use default rounding mode
@JeffBezanson JeffBezanson merged commit ff9cebe into JuliaLang:master Dec 22, 2014
@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2016

@simonbyrne sorry to bump a 2-year-old PR, but do you have any thoughts whether the loosening here of what used to be

function round{T<:Integer}(::Type{T}, x::Rational)

to

round{T}(::Type{T}, x::Rational)

was intentional? So it's been possible since this PR to do round(Float32, 2//3) but not round(Float32, 0.7). This came up in #19791 (comment), also at #19791 (comment)

@simonbyrne simonbyrne deleted the roundeven branch December 31, 2016 12:16
@simonbyrne
Copy link
Contributor Author

I don't think so, as there are no tests for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants