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

make BigFloat(x::BigFloat) respect global precision (fix #20766) #29127

Merged
merged 2 commits into from Oct 26, 2018

Conversation

5 participants
@rfourquet
Copy link
Contributor

commented Sep 11, 2018

This makes e.g. BigFloat(BigFloat(1), 128) have precision 128 instead of the default 256.

I'm not familiar with these questions, but this fix seems relatively sane to me. Currently there is no easy way (AFAIK) to create a BigFloat out of another one with a different precision, so allowing the documented API (BigFloat(x, prec)) to do what it says seems to be a must. On the other hand, I don't know whether it's officially documented that BigFloat(x) creates a BigFloat with precision(BigFloat) precision. But it's what we do for all other numbers, so I believe this is the most expected behavior to implement also for BigFloat.

As raised by @simonbyrne in the issue, there is also the question of convert(BigFloat, x::BigFloat), which I didn't touch for now in this PR. But I think we don't have the expectation that convert will create a new object (unlike for constructors), only that it returns an object of the destination type.

It seems slighly breaking, but on the other hand, it was labeled as "bug" in the issue, so...

@rfourquet rfourquet force-pushed the rf/bigfloat-prec branch from 8089f56 to 0e16cfc Sep 11, 2018

@StefanKarpinski StefanKarpinski requested a review from simonbyrne Sep 11, 2018

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

I think BigFloat(x, prec) should definitely make a new BigFloat with the given precision, but BigFloat(x) probably should not change x. I don't know what would break, but it seems like a gotcha for conversion and construction to be different for some number types. (Clarification: conversion and construction are different in general, but not for number types.)

@simonbyrne

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

I think it should be fine: BigFloat is a bit of a weird number type in any case.

@simonbyrne

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

@dpsanders @JeffreySarnoff What are your thoughts?

@dpsanders

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Yes, definitely -- have been wanting for a long time!

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

Ok, I'll defer to you folks on this, but then writing T(x) has to come with the caveat that "T might be weird" :)

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

My slight preference to have BigFloat(x) using the global precision is that otherwise there is this other gotcha: you have a bunch of numbers in an array a and want to convert them to big floats, e.g. via BigFloat.(a), and expect to get homogeneous precisions, which works until one BigFloat happens to already be in a.

@JeffreySarnoff

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

@JeffBezanson When you say BigFloat(x) should not change x -- for clarification
(a) BigFloat(x::BigFloat) == x # precision of x is unchanged
(b) BigFloat(x::!BigFloat) == BigFloat(x, prec=what?)

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

I meant BigFloat(x::BigFloat) == x.

@JeffreySarnoff

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

👍 I am for that (I do the same with all my numeric types).

@rfourquet We have BigFloat.(x::Array, prec) to make each element be of the same precision and that better fits with other aspects of the language (round comes to mind). BigFloat(x; precision=prec) would be even more fitting, I suppose.

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

this other gotcha: you have a bunch of numbers in an array a and want to convert them to big floats, e.g. via BigFloat.(a), and expect to get homogeneous precisions

That's not so obvious to me. What if some of the numbers are already BigFloats with more precision than the current global setting? Is the intent of BigFloat.(a) really to lose precision?

Anyway, I think one thing we can all agree on is that it's not easy for everybody to be happy as long as this is a global setting.

@JeffreySarnoff

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

<and the change to a non-global setting is reasonably easy b'kuz the library's underlying struct used when we construct a BigFloat carries precision as a field>

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2018

We have BigFloat.(x::Array, prec) to make each element be of the same precision

This works indeed, but this is nonetheless a gotcha. Say you use the setprecision function, then one definitely expects to get the requested precision, this is documented as such. For example:

setprecision(52) do
    for x in A
        z = BigFloat(x)
        p = prevfloat(z) # now, z and p don't have the same precision if `x` was already a BigFloat :-/
        @assert z == nextfloat(p) # ouch!
    end
end

Is the intent of BigFloat.(a) really to lose precision?

I wouldn't say it's the intent, but a possible consequence of the meaning I wish BigFloat has, i.e. that BigFloat.(a) produces numbers with all the same precision. There will be the same problem when converting from other floating points with more precision than the current precision(BigFloat).

@JeffBezanson

This comment has been minimized.

Copy link
Member

commented Sep 12, 2018

Ok, well, I will go away and defer to the more numerical folks here :)

@JeffreySarnoff

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

I don't see the gotcha. Let's separate concerns. BigFloat(x::BigFloat) is x unchanged will save many unintended allocations, and simplify logic given the ease with which one can mix precisions unintentionally.

To produce numbers of some same precision BigFloat.(x::T, precision) where T<:BigFloat should work -- and that needs to be fixed. It should not stop this change, though -- it could accompany it.

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2018

and simplify logic given the ease with which one can mix precisions unintentionally.

If you look at my last example, it's precicely a case where precision is mixed unintentionally ! i.e. BigFloat(x) == prevfloat(nextfloat(BigFloat(x)) depends on whether x isa BigFloat.

For the record, I made this PR because I got caught by the "gotcha" of expecting that setprecision is a contract to have any function creating a BigFloat set its precision to the specified one (same contract as for the BigFloat(x, prec) constructor).

@simonbyrne

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2018

I think this is ready to merge, unless there are more objections.

@simonbyrne

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

@rfourquet can you add an item to NEWS.md?

@rfourquet

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

can you add an item to NEWS.md?

OK, I will just wait for #29490 first, which prepares this file for 1.1.0.

@rfourquet rfourquet force-pushed the rf/bigfloat-prec branch from 0e16cfc to fc7f70a Oct 4, 2018

@simonbyrne simonbyrne merged commit c887c74 into master Oct 26, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
julia freebsd ci Build done
Details

@simonbyrne simonbyrne deleted the rf/bigfloat-prec branch Oct 26, 2018

@simonbyrne

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.