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

std.bigint.BigInt: allow casting to floating point numbers #7151

Merged
merged 1 commit into from Dec 29, 2019

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Aug 20, 2019

Limitation: casting to & comparison with quadruple precision floating point numbers are not supported. The path for 80 bit (EDIT: and greater) extended floats use std.math.scalbn which currently is impure and non-CTFE-able.

@dlang-bot
Copy link
Contributor

dlang-bot commented Aug 20, 2019

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
20146 enhancement Allow casting from std.bigint.BigInt to built-in floating point types

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7151"

@n8sh n8sh force-pushed the issue-20146-20147 branch 5 times, most recently from d99bde8 to 687a993 Compare August 21, 2019 10:12
@Biotronic
Copy link
Contributor

Is scalbn a better solution than r * 2.0L^^N? At the very least, that solution should be a good enough stopgap for CTFE.

@n8sh
Copy link
Member Author

n8sh commented Aug 23, 2019

@Biotronic: let's use that to make scalbn CTFE-able in general (#7153).

@n8sh n8sh force-pushed the issue-20146-20147 branch 4 times, most recently from c4f985c to 7067d48 Compare August 26, 2019 20:05
@n8sh
Copy link
Member Author

n8sh commented Aug 26, 2019

Added path for arbitrarily high precision floating point numbers. Locally tested that it works for 80 bit reals.

@n8sh n8sh force-pushed the issue-20146-20147 branch 5 times, most recently from 9df8cdc to 26cd036 Compare August 26, 2019 21:59
@n8sh
Copy link
Member Author

n8sh commented Aug 26, 2019

I may have to rethink the rounding used in opCast. When unsigned magnitude equals or exceeds 2^^64 casting to floating point truncates (rounds towards zero) because that makes it easy to use opCast in the implementation of opCmp. But when magnitude is less than 2^^64 rounding is performed as if converting a ulong to floating point.

@n8sh n8sh force-pushed the issue-20146-20147 branch 3 times, most recently from 98f5d9b to 453575c Compare August 26, 2019 23:30
@n8sh
Copy link
Member Author

n8sh commented Aug 26, 2019

I've changed opCast to specify the rounding mode. Now the default rounding is roundToNearest but opCmp calls it with roundToZero.

@n8sh n8sh force-pushed the issue-20146-20147 branch 2 times, most recently from 9f28551 to e27af58 Compare August 26, 2019 23:57
@n8sh n8sh closed this Aug 27, 2019
@n8sh n8sh reopened this Aug 27, 2019
@n8sh n8sh added the WIP Work In Progress - not ready for review or pulling label Aug 27, 2019
@n8sh n8sh force-pushed the issue-20146-20147 branch 10 times, most recently from f1e4a0e to 221ce25 Compare September 4, 2019 18:57
std/bigint.d Outdated
// If we wanted to round ties away from 0 we could just add
// roundUpInc to sansExponent. The more complicated logic is
// so ties are rounded to even, which matches the default
// rounding behavior when casting int/long to float/double.
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this seemingly isn't the default rounding mode on the Win32 & FreeBSD_32 test machines, my inclination is to abandon this rounding mode and round ties away from zero for float/double.

@n8sh n8sh changed the title std.bigint.BigInt: allow casting to & comparison with floating point numbers std.bigint.BigInt: allow casting to floating point numbers Sep 8, 2019
@n8sh n8sh added WIP Work In Progress - not ready for review or pulling and removed WIP Work In Progress - not ready for review or pulling labels Sep 8, 2019
@ghost
Copy link

ghost commented Dec 28, 2019

Is this still WIP?

@dlang-bot dlang-bot merged commit c37c4bc into dlang:master Dec 29, 2019
@n8sh
Copy link
Member Author

n8sh commented Jan 2, 2020

@berni44 the WIP tag is because the rounding behavior might astonish someone who used it without reading the documentation.

// BigInts that fall exactly between two non-infinite floats/doubles
// are rounded away from zero when cast to float/double. (Note that
// in most environments this is NOT the same rounding rule rule used
// when casting int/long to float/double.)

Any way of handling rounding had disadvantages. I'm not convinced that this choice is the best.

@n8sh
Copy link
Member Author

n8sh commented Jan 2, 2020

Also, this isn't great:

When cast to float or double or 64-bit real the rounding is strictly defined. When cast to extended-precision real the rounding rules vary by environment.

@ghost
Copy link

ghost commented Jan 2, 2020

@berni44 the WIP tag is because the rounding behavior might astonish someone who used it without reading the documentation.

I had exactly the same problems with the functions that print floating point numbers in std.format. But there is a slight difference: Printing is IMHO mainly done for humans and there is IMHO "ties away from zero" the expected choice. Here, the numbers might be used to do some more calculating and as far as I know "ties to even" works better for that, because it can straighten out the accumulating errors.

What I currently do in std.format is to read out the rounding mode of the processor and use this mode (round down, round up, round to zero, round to nearest with ties to even). That's what printf does and the reason why printing floats isn't pure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Enhancement WIP Work In Progress - not ready for review or pulling
Projects
None yet
4 participants