# Add nextfloat(::BigFloat, n), prevfloat(::BigFloat,n) #31310

Merged
merged 6 commits into from Apr 1, 2019

## Conversation

Contributor

### narendrakpatel commented Mar 10, 2019

 New constructor for BigFloat. refer this Improve nextfloat/prevfloat and introduce nextfloat! and prevfloat! Introduce nextfloat(::BigFloat, n) and prevfloat(::BigFloat, n) Tests for constructor, nextfloat and prevfloat Closes: #31276

### fredrikekre requested a review from simonbyrneMar 11, 2019

reviewed
 end nextfloat(x::BigFloat, n::Int) = nextfloat!(BigFloat(x, new=true), n)

#### StefanKarpinski Mar 11, 2019

Member

Technically when `n == 0` we could just return `x`. Not sure it's worth the complexity though.

#### narendrakpatel Mar 11, 2019

Author Contributor

Actually, the special case `n==0` is already considered in `nextfloat!()`. Note that `nextfloat()` had to return a copy of x, so anyhow we would need to call Bigfloat constructor. Thus no special case was specifically created for this function.

#### StefanKarpinski Mar 11, 2019

Member

Actually, the special case `n==0` is already considered in `nextfloat!()`

But that's too late: `BigFloat(x, new=true)` has already allocated a new copy of `x` by the time `nextfloat!` gets the value.

#### narendrakpatel Mar 11, 2019

Author Contributor

My argument is that, nextfloat() needs to return a new copy of x, as it does not ends with `!`. So even in case n==0, we would need to create a copy of x. Thus, no computation is wasted.

#### StefanKarpinski Mar 11, 2019

Member

My argument is that, `nextfloat()` needs to return a new copy of `x`

Returning the same value is not a mutating operation. Since BigFloats are considered to be immutable it's fine to return the same value instead of allocating a new identical value.

#### narendrakpatel Mar 13, 2019

Author Contributor

Does this look good?

`nextfloat(x::BigFloat, n::Int) = n == 0 ? x : nextfloat!(BigFloat(x, new=true), n)`

#### StefanKarpinski Mar 13, 2019

Member

Yes, that's just it.

Member

### StefanKarpinski commented Mar 11, 2019

 This is looking fairly good. There's a key API question in here: do we want `BigFloat(x, new=true)` to be the official way to ask for a newly allocated big float?
Contributor Author

### narendrakpatel commented Mar 11, 2019

 We had a long discussion on what the API of the BigFloat should look like here. I believe the summary of the whole discussion can be found here. @JeffreySarnoff can elaborate more. What are your views on it?
Member

### StefanKarpinski commented Mar 11, 2019

 That's fine, I'm calling broader attention to it to get some agreement before merging.

### vtjnash added decision triage labels Mar 11, 2019

reviewed
 return z end function BigFloat(x::BigFloat, r::MPFRRoundingMode=ROUNDING_MODE[]; precision::Integer=MPFR.precision(x), new::Bool=false)

#### simonbyrne Mar 11, 2019 • edited

Contributor
Suggested change
 precision::Integer=MPFR.precision(x), new::Bool=false) precision::Integer=MPFR.precision(x), new::Bool=precision!=MPFR.precision(x))
reviewed
 end function BigFloat(x::BigFloat, r::MPFRRoundingMode=ROUNDING_MODE[]; precision::Integer=MPFR.precision(x), new::Bool=false) precision == MPFR.precision(x) && !new && return x

#### simonbyrne Mar 11, 2019

Contributor
Suggested change
 precision == MPFR.precision(x) && !new && return x new || return x

#### narendrakpatel Mar 11, 2019

Author Contributor

Does this sound good?

```precision != MPFR.precision(x) && !new &&
@warn "new=false is ignored because precision differ" &&
return BigFloat(x, precision=precision)
new || return x```

#### StefanKarpinski Mar 12, 2019

Member

We definitely don't want random warnings in unexpected corner cases. If you called `BigFloat(x)` and didn't ask for a definitely new value it should be fine to either give you a new `BigFloat` or to give you the same one since the official API for BigFloats is that they are immutable, so you shouldn't be able to tell the difference anyway.

The only legitimate use case for `BigFloat(x, new=true)` is when you're copying an input from somewhere and then are going to mutate that input in the internal implementation of an algorithm. That's why I'm not convinced about this API: if you wanted a new copy of an incoming value so that you can mutate it, wouldn't you also want to copy the precision?

#### narendrakpatel Mar 12, 2019

Author Contributor

The precision is being copied. In the constructor, default value of precision is precision(x).

#### StefanKarpinski Mar 12, 2019

Member

Ok, then with @simonbyrne's new default value I'm more ok with this.

#### JeffreySarnoff Mar 13, 2019

Contributor

We should decide about using `new` as a parameter first. imo my prior note gives a way to determine that which strips away some of the dross.

#### JeffreySarnoff Mar 14, 2019 • edited

Contributor

I think this does what we want and avoids the controversial.

defined in mpfr and not exported, these respect the precision of `x`

``````function duplicate(x::BigFloat)
z = BigFloat(; precision = precision(x))
ccall((:mpfr_set, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, Int32), z, x, 0)
return z
end

function nextfloat!(x::BigFloat)
ccall((:mpfr_nextabove, :libmpfr), Int32, (Ref{BigFloat},), x) != 0
return x
end

function prevfloat!(x::BigFloat)
ccall((:mpfr_nextbelow, :libmpfr), Int32, (Ref{BigFloat},), x) != 0
return x
end
``````

defined in mpfr.jl and exported, these also respect the precision of `x`
without influencing any other aspects of the `BigFloat` interface.

``````nextfloat(x::BigFloat) = nextfloat!(duplicate(x))
prevfloat(x::BigFloat) = prevfloat!(duplicate(x))
``````

`nextfloat(x,n), prevfloat(x,n), nextfloat!(x,n), prevfloat!(x,n)` accordingly

#### JeffreySarnoff Mar 14, 2019 • edited

Contributor

^ revised so nextfloat(x) != x

#### StefanKarpinski Mar 14, 2019

Member

+💯 to the non-controversial approach of introducing an unexported `duplicate` function for this PR. That let's us get this PR merged adding the desired `nextfloat` and `prevfloat` methods and we can have a separate discussion of the best API for creating copies.

#### JeffreySarnoff Mar 14, 2019 • edited

Contributor

@narendrakpatel please ^, do as we discussed last night.
Don't worry about the failing tests now -- a closer look shows your PR does not trigger the fails.

referenced this pull request Mar 14, 2019
Member

### Keno commented Mar 14, 2019

 We took a brief look at this during triage. Two comments: We don't like the `new` keyword argument, but that seems to have been discussed extensively already. The other observation is that this seems like it changes the rounding mode behavior of the nextfloat/prevfloat functions and is thus a breaking change.
Contributor Author

### narendrakpatel commented Mar 14, 2019 • edited

 @Keno If we implement the `_duplicate` like this then I think that issue will be solved. ```function _duplicate(x::BigFloat) z = BigFloat(;precision=precision(x)) ccall((:mpfr_set, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, MPFRRoundingMode), z, x, ROUNDING_MODE[]) return z end``` Basically only change in proposed implementation would be the use of `z=BigFloat(x, precision=precision(x))` instead of `z=BigFloat(x)` in nextfloat/prevfloat functions. Shall I make these changes in the PR?
Contributor

### JeffreySarnoff commented Mar 14, 2019

 @narendrakpatel Because there is now exported `nextfloat(::BigFloat)` and `prevfloat(::BigFloat)` and they use `precision(BigFloat)` rather than the precision of their argument, to do anything other than that in implementing `nextfloat(::BigFloat, ::Int)` and `prevfloat(::BigFloat, ::Int)` would require (for logical consistancy) that the current `nextfloat` and `prevfloat` implementations change to use the precision of their arguments. That change would be "breaking" -- so it is unavailable until Julia v2. This means that the implementation of `nextfloat(::BigFloat, ::Int)` and `prevfloat(::BigFloat, ::Int)` must be limited to the equivalent of repeatedly calling the currently exported versions `nextfloat(::BigFloat)` and `prevfloat(::BigFloat)`. All of the good thought and refinement that we have developed over the past days will be useful. Either as contributing to the "reform" of BigFloats for v2, or in providing important principles for a complementary package, say `LargeFloats`. I know others are paying attention to what is and is not "breaking". It's most important to the viability of Julia as the user base grows. My perspective is more about where we might be going, or how I might go where I like with Julia. So .. given this, why not take a shot at cleanly coding the "works like repeated calls to _" versions of these functions for `BigFloat`.
Contributor

### JeffreySarnoff commented Mar 14, 2019

 Is that ^ where we are at, @Keno?
Member

### Keno commented Mar 14, 2019

 I'm just the scribe here ;). If `nextfloat(::BigFloat, n)` didn't work before, it can do whatever, we just can't change any behavior that worked before (if we think it's minor, we can make that decision and put it in a minor version, but that would need to be explicitly decided and called out).
Contributor

### JeffreySarnoff commented Mar 14, 2019

 As far as I know, nobody uses `prevfloat(BigFloat)`, `nextfloat(BigFloat)` -- perhaps there is someone who once did something with that -- but the way people have related to BigFloats is not one that naturally leads to thinking of their previous and next floating point values. Perhaps the best evidence of this is that until I noticed it, there were no mentions of the lack of an nth next or nth prev. There is a good deal of support for the PR as @narendrahkpatel is about to provide. @narendrakpatel Now Is The Time.

Contributor

### JeffreySarnoff commented Mar 15, 2019

 Until v1.1, there was no `precision` keyword to use constructing a `BigFloat`. There was an unnamed positional arg which would pass along a value for the precision, which was of heroically little use because as soon as on did anything numerical with a `BigFloat`, the precision of the result has been slammed with the current value of `precision(BigFloat)`. Other than looking at digits, using the unnamed second arg for setting custom precision has not been generally helpful -- and so, has not been seen as a tool of the BigFloat realm. That `nextfloat` and `prevfloat` stepped bits at the current value of `precision(BigFloat)` is exactly how they will behave going forward when given a BigFloat that has been constructed using the current `precision(BigFloat)`. Which is exactly as they have behaved when given a BigFloat that has been constructed using the current `precision(BigFloat)` . (a) there is no pattern of use for `nextfloat, prevfloat` with BigFloats because the question of precision has lacked resonance beyond `256 bits, that's good` maybe for this `128 bits` or for that `1024 bits` (b) the sort of numerical processing that benefits from adjusting the precision does not work very well when the number type keeps slamming the current precision onto old and new results alike. That `nextfloat` and `prevfloat` will respect the precision of the values given means they will work just as they have worked (had they been used) as the values given had been of the current precision (had they been given). Allowing more precise and well-mannered numerical exploration going forward is only upside.
Contributor Author

### narendrakpatel commented Mar 19, 2019

 @StefanKarpinski @Keno Does this PR require any other changes?

### rfourquet added the bignums label Mar 20, 2019

reviewed
 ccall((:mpfr_nextabove, :libmpfr), Int32, (Ref{BigFloat},), z) != 0 return z function nextfloat!(x::BigFloat, n::Integer) n==0 && return x

#### rfourquet Mar 20, 2019

Contributor

For style consistency, it seems better to add space around `==` (same for `prevfloat!`).

#### rfourquet Mar 20, 2019

Contributor

But actually, this line seems unnecessary, the same result is achieved by an empty `for`-loop below.

 nextfloat(x::BigFloat, n::Integer) = n==0 ? x : nextfloat!(_duplicate(x), n) prevfloat(x::BigFloat, n::Integer) = n==0 ? x : prevfloat!(_duplicate(x), n) nextfloat!(x::BigFloat) = (ccall((:mpfr_nextabove, :libmpfr), Int32, (Ref{BigFloat},), x); x)

#### rfourquet Mar 20, 2019

Contributor

Why not instead add a default argument, like `nextfloat!(x::BigFloat, n::Integer=1)` ?

#### JeffreySarnoff Mar 20, 2019

Contributor

I think that does not decomplexify; and since `nextfloat!` is being introduced and not exported .. keeping it as simple as possible lets the exported function be more reliably maintained.

#### rfourquet Mar 20, 2019

Contributor

Well, I proposed this change precisely because that seems simpler to me, since default args are a well understood feature, and this allows to merge two definitions into one, with easier maintainability IMHO.

#### JeffreySarnoff Mar 20, 2019

Contributor

I missed some of your earlier comment. You are talking about giving `n` the default value of `1`. I'd have no objection to that in any of the four functions: `nextfloat!`, `prevfloat!`, `nextfloat`, `prevfloat` were it not the case that `float.jl` does not give `nextfloat`, `prevfloat` default values. I'm sure that's at least in part because `float.jl` was written eariler. Nonetheless, there is a proclivity to follow the conventions that are in use.

#### rfourquet Mar 20, 2019

Contributor

float.jl does not give nextfloat, prevfloat default values.

My guess would be that it's because those functions are documented separately. By the way, the docstring of `nextfloat(x, n)` should be updated here to include `BigFloat` (or simply to remove the type constraint `IEEEFloat`, which is not even exported or documented).

proclivity

nice new word learnt, thanks!

#### rfourquet Mar 20, 2019

Contributor

We can do the same thing here

We can also do it differently, this is what default arguments are for.

#### JeffreySarnoff Mar 20, 2019

Contributor

Why is it a breaking change -- what is the rounding now?

#### narendrakpatel Mar 20, 2019

Author Contributor

Currently, we can do `setrounding(T, Mode)` and the `nexfloat()` uses that Mode. What is implemented in this PR is RoundNearest Mode.
I would prefer the method mentioned by @rfourquet but would not like to break the code style.

#### JeffreySarnoff Mar 20, 2019 • edited

Contributor

Yes, however when there is no `setrounding` set, the default is `RoundNearest`. So its not clear to me that using `RoundNearest` is breaking anything. If `setrounding` sets rounding to `RoundDown` or to `RoundUp` what difference does it make -- the nextfloat is the same as the nextfloat rounded down and the same as the nextfloat rounded up.
Seems to me that by definition `nextfloat(x) != x` for finite(x) and similarly for `prevfloat` .. if they cannot be equal and presuming that `nextfloat(prevfloat(x)) == x` then it is RoundNearest whatever you call it.

#### rfourquet Mar 22, 2019

Contributor

I would prefer the method mentioned by @rfourquet but would not like to break the code style.

I wonder why you would think that this would break code style. It would rather be your current version which breaks style. If there is no speed advantage (which I doubt in this case) to having two separate bodies with repetition, just merge it into one function with default argument.

Contributor

### rfourquet commented Mar 20, 2019

 This new implementation of `nextfloat(::BigInt)` seems more useful to me, and I already considered proposing such a change: indeed it's difficult for me to imagine a use-case for `nextfloat(x)` which doesn't have the same precision as `x`. On the other hand, the current behavior is the one which is consistent with `setprecision`, so it's not an obvious choice. Anyway, I say that just to suggest that explicit documentation of this corner case (deviation from the consistency of `setprecision`) should be added to this PR.
reviewed
Show resolved Hide resolved
Contributor

### JeffreySarnoff commented Mar 22, 2019 • edited

 We have reached a meeting of appropriateness (absence of dissent, presence of consent), right? right. @narendrakpatel has been diligent in his incorporation of the best recentest both in test and in src. Let's clear this PR into Base.
Contributor

### rfourquet commented Mar 22, 2019

 @narendrakpatel has been diligent in his incorporation of the best recentest both in test and in src. Let's clear this PR into Base. I wish some more tests and documentation like we suggested, and ideally an answer to my comments. But more importantly, regarding the breaking change, an explicit decision should be taken...
Contributor

### JeffreySarnoff commented Mar 22, 2019

 @narendrakpatel I thought you had added those tests already -- please do. And the docs modification too. @rfourquet What is the breaking change -- how does the rounding mode break anything if the result of `nextfloat` `prevfloat` is isomorphic to computation under `RoundNearest` whatever the RoundingMode setting?
Contributor

### rfourquet commented Mar 22, 2019

 What is the breaking change See my previous post, the current behavior of `nextfloat` is to respect `setprecision`, i.e. `nextfloat(x)` doesn't necessarily create a float with the same precision as `x`. I am personally in favor of the proposed change, but this is breaking; so it must be decided whether we want this change, and if yes, whether this is a "minor change".
Contributor

### JeffreySarnoff commented Mar 28, 2019

 supergood -- getting all newsy

### JuliaLang deleted a comment from JeffreySarnoffMar 28, 2019

``` implement internal function _duplicate ```
``` 753096c ```
``` implement nextfloat, prevfloat, nextfloat(x,n), prevfloat(x,n) ```
``` 01c0a58 ```
``` add tests for new implementation of nextfloat/prevfloat ```
``` 49541cd ```
``` update implementation and tests for nextfloat/prevfloat ```
``` 562567c ```
``` update docs for nextfloat/prevfloat ```
``` e23d59d ```

### narendrakpatelforce-pushed the narendrakpatel:nextfloat-prevfloat branch from `c32800d` to `9db7274`Mar 28, 2019

approved these changes
Contributor

### JeffreySarnoff commented Mar 28, 2019 • edited

 There is a typo and an inoperative link in the News item: New methods `nexfloat(::BigFloat, n::Int)` and `prevfloat(::BigFloat, n::Int)` are introduced. ([#31310]) ⬳ omitted the 't' in 'nextfloat' (reworded to conform to the style of related News items) `nextfloat(::BigFloat, n::Int)` and `prevfloat(::BigFloat, n::Int)` have now been added. (#31310) Be careful to copy the full link into News: in html it looks like this: (< a href="https://github.com/JuliaLang/julia/issues/31310">#31310) to appear as this: (#31310) @narendrakpatel please make correction and then notify me

### narendrakpatelforce-pushed the narendrakpatel:nextfloat-prevfloat branch from `9db7274` to `d43ea31`Mar 29, 2019

approved these changes
Contributor

### JeffreySarnoff commented Mar 29, 2019

 @narendrakpatel On behalf of the other participants, thank you for this awesome first PR.
Contributor Author

### narendrakpatel commented Mar 29, 2019

 If this is okay, can this be merged? @StefanKarpinski @simonbyrne
reviewed
NEWS.md Outdated Show resolved Hide resolved
reviewed
NEWS.md Outdated
 * `inv(::Missing)` has now been added and returns `missing` ([#31408]). * `inv(::Missing)` has now been added and returns `missing` ([#31451]). * `nextfloat(::BigFloat, n::Int)` and `prevfloat(::BigFloat, n::Int)` have now been added

#### rfourquet Mar 29, 2019

Contributor

nitpick, if you happen to re-push before merge: `n::Integer`

#### StefanKarpinski Mar 29, 2019

Member
Suggested change
 * `nextfloat(::BigFloat, n::Int)` and `prevfloat(::BigFloat, n::Int)` have now been added * `nextfloat(::BigFloat, n::Integer)` and `prevfloat(::BigFloat, n:: Integer)` methods have been added

#### JeffreySarnoff Mar 29, 2019

Contributor

@narendrakpatel is in class and will push this modification when he gets home.

#### JeffreySarnoff Mar 29, 2019

Contributor

or -- just go ahead and make the change -- we all agree

#### narendrakpatel Mar 29, 2019

Author Contributor

I am back. I will make the changes right away.

#### JeffreySarnoff Mar 29, 2019

Contributor

do you know how to "squash"? I don't.

#### narendrakpatel Mar 29, 2019

Author Contributor

For now, I will just use `git commit --amend` but you can squash commit in the following way:

• `git rebase -i HEAD~n`
• replace `pick` with `squash`

#### StefanKarpinski Mar 29, 2019

Member

You don't have to do anything. It can be squashed by the person doing the merge.

#### StefanKarpinski Mar 29, 2019

Member

(But yes, it can be done by you as well with the appropriate git incantations.)

Member

### StefanKarpinski commented Mar 29, 2019 • edited

 Looks good modulo doc suggestions. Please squash when merging.

### narendrakpatelforce-pushed the narendrakpatel:nextfloat-prevfloat branch from `d43ea31` to `899d69d`Mar 29, 2019

Contributor Author

### narendrakpatel commented Mar 29, 2019

 @StefanKarpinski I made all the changes that are recommended.
reviewed
Show resolved Hide resolved
reviewed
NEWS.md Outdated
 @@ -49,6 +52,9 @@ Standard library changes * A no-argument construct to `Ptr{T}` has been added which constructs a null pointer ([#30919]) * `strip` now accepts a function argument in the same manner as `lstrip` and `rstrip` ([#31211]) * `mktempdir` now accepts a `prefix` keyword argument to customize the file name ([#31230], [#22922]) * `nextfloat(::BigFloat)` and `prevfloat(::BigFloat)` now returns a value with the same precision as their argument, which means that (in particular) `nextfloat(prevfloat(x)) == x` whereas

#### StefanKarpinski Mar 29, 2019

Member
Suggested change
 as their argument, which means that (in particular) `nextfloat(prevfloat(x)) == x` whereas as their argument, which means that (in particular) `nextfloat(prevfloat(x)) == x` whereas
reviewed
NEWS.md Outdated
 @@ -49,6 +52,9 @@ Standard library changes * A no-argument construct to `Ptr{T}` has been added which constructs a null pointer ([#30919]) * `strip` now accepts a function argument in the same manner as `lstrip` and `rstrip` ([#31211]) * `mktempdir` now accepts a `prefix` keyword argument to customize the file name ([#31230], [#22922]) * `nextfloat(::BigFloat)` and `prevfloat(::BigFloat)` now returns a value with the same precision as their argument, which means that (in particular) `nextfloat(prevfloat(x)) == x` whereas previously this could result in a completely different value with a different precision ([#31310])

#### StefanKarpinski Mar 29, 2019

Member
Suggested change
 previously this could result in a completely different value with a different precision ([#31310]) previously this could result in a completely different value with a different precision ([#31310])

#### narendrakpatel Mar 29, 2019

Author Contributor

It is just a markdown thing. I had `build` the docs and checked that it looks fine. Do you still want me to push to these changes?

#### StefanKarpinski Mar 29, 2019

Member

Probably not worth it, although it does match the way the rest of the file is formatted. You can also just merge the suggested changes through the GitHub UI.

#### narendrakpatel Mar 29, 2019

Author Contributor

Changed the files.

Member

### StefanKarpinski commented Mar 29, 2019

 Ok, we're converging here. Now it's just the rebase artifact and markdown indentation.
``` update news ```
``` 2d053f9 ```

Member

### StefanKarpinski commented Mar 29, 2019

 This is ready to be squash-merged as soon as tests pass. Great work, @narendrakpatel—thanks for sticking it out to the end!
Contributor Author

### narendrakpatel commented Mar 31, 2019

 If this is okay, can this be merged? @StefanKarpinski PS: some tests are failing but the reasons are not related to this issue.

### rfourquet merged commit `4c2e570` into JuliaLang:master Apr 1, 2019 8 of 12 checks passed

#### 8 of 12 checks passed

buildbot/package_freebsd64 Run complete
Details
buildbot/package_linuxppc64le Run complete
Details
buildbot/tester_linuxaarch64 Run complete
Details
buildbot/tester_win32 Run complete
Details
buildbot/package_linux32 Run complete
Details
buildbot/package_linux64 Run complete
Details
buildbot/package_linuxaarch64 Run complete
Details
buildbot/package_win32 Run complete
Details
buildbot/tester_linux32 Run complete
Details
buildbot/tester_linux64 Run complete
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Member

### StefanKarpinski commented Apr 1, 2019

 Exciting! Congrats on the PR, @narendrakpatel!