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

Added Float32 and Float64 consts #4787

Merged
merged 6 commits into from Jun 5, 2018
Merged

Conversation

konovod
Copy link
Contributor

@konovod konovod commented Aug 4, 2017

Fixes #4785
Adds EPSILON, DIGITS, RADIX, MANT_DIG, MIN_EXP, MAX_EXP, MIN_10_EXP, MAX_10_EXP and MIN_POSITIVE consts to Float32 and Float64.
I've renamed DIG -> DIGITS as it imho looks clearer this way. For a ROUNDS - I don't know how to get its value and is it really needed, so skipped it.
If the constants are different for some platforms, they of course should be moved to platform-specific files, but i think they are currently same for all supported platforms.

@konovod
Copy link
Contributor Author

@konovod konovod commented Aug 4, 2017

Huh, travis fails on 32 bits with strange error. No idea what could i broke (and no 32-bit machine to test) - Float64::MAX and Float64::MIN are used only in benchmark/ips.cr and changing MAX/MIN to the finite value is the only potentially breaking change.

@ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Aug 4, 2017

Try to not change MIN and MAX.

@konovod
Copy link
Contributor Author

@konovod konovod commented Aug 4, 2017

Still broken on 32-bits. No ideas.

@konovod
Copy link
Contributor Author

@konovod konovod commented Aug 5, 2017

Well, working now. I'll try to revert MAX/MIN back to finite values.

@konovod konovod force-pushed the float_consts branch 2 times, most recently from 22ff674 to 412b508 Compare Aug 5, 2017
@konovod
Copy link
Contributor Author

@konovod konovod commented Aug 5, 2017

Reverted, CI is green 😄

@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 5, 2017

It would be great if you could specify the float constants as hexadecimal then use unsafe_as to convert them to floats. That way we know we're not loosing precision.

@konovod
Copy link
Contributor Author

@konovod konovod commented Aug 5, 2017

@RX14 in that case, should there also be some specs ensuring that 0x34000000_u32.unsafe_as(Float32) is actually small number?

@RX14
Copy link
Contributor

@RX14 RX14 commented Aug 5, 2017

@konovod yes.

@konovod
Copy link
Contributor Author

@konovod konovod commented Aug 5, 2017

done, but there is an error on 32-bits travis again. If it happens on other PRs too it's ok (well, not ok, but unrelated), but if only on this - perhaps there is some problem with it?

@wontruefree
Copy link
Contributor

@wontruefree wontruefree commented May 26, 2018

@konovod what is the status of this PR?

@konovod
Copy link
Contributor Author

@konovod konovod commented May 26, 2018

I don't see anything preventing it from merge. Travis 32-bit was failing with apparently irrelevant error, perhaps will be green if restarted. But i forgot how to restart CI - should I force push the same commit?

Copy link
Contributor

@RX14 RX14 left a comment

Perhaps the specs should be the other way around: specify the real numbers as constants then check them against the hex values in the specs.

src/float.cr Outdated
# value is
EPSILON = 0x34000000_u32.unsafe_as(Float32) # 1.19209290e-07_f32
# The number of decimal digits that can be represented without losing precision
DIGITS = 15
Copy link
Contributor

@RX14 RX14 May 27, 2018

Choose a reason for hiding this comment

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

This should be 6.

Copy link
Contributor Author

@konovod konovod May 28, 2018

Choose a reason for hiding this comment

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

nice catch, my bad!

@wontruefree
Copy link
Contributor

@wontruefree wontruefree commented May 27, 2018

This might sounds like a dumb question but are these set at compile time or runtime? seem to me things like unsafe_as would be runtime. If this is the case should these be changes to compile time? Or does it even matter?

@konovod
Copy link
Contributor Author

@konovod konovod commented May 28, 2018

Even if unsafe_as isn't optimized away, consts initialization happens only at start of program, so performance doesn't matter. Anyway, I've changed it to the way @RX14 suggested, so unsafe_as is now only in specs.

src/float.cr Outdated
# value is
EPSILON = 1.19209290e-07_f32
# The number of decimal digits that can be represented without losing precision
DIGITS = 15
Copy link
Contributor

@Sija Sija May 28, 2018

Choose a reason for hiding this comment

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

Still 6 :)

Copy link
Contributor Author

@konovod konovod May 29, 2018

Choose a reason for hiding this comment

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

huh

RX14
RX14 approved these changes May 29, 2018
src/float.cr Outdated
# Largest finite value
MAX = 3.40282347e+38_f32
# The machine epsilon (difference between 1.0 and the next representable value)
# value is
Copy link
Contributor

@luislavena luislavena May 29, 2018

Choose a reason for hiding this comment

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

Probably leftover from a change?

@RX14 RX14 requested a review from bcardiff Jun 2, 2018
@RX14 RX14 added this to the 0.25.0 milestone Jun 2, 2018
@bcardiff bcardiff merged commit 85c26da into crystal-lang:master Jun 5, 2018
4 checks passed
@bcardiff
Copy link
Member

@bcardiff bcardiff commented Jun 5, 2018

Thanks @konovod !

@konovod konovod deleted the float_consts branch Jun 5, 2018
chris-huxtable pushed a commit to chris-huxtable/crystal that referenced this issue Jun 6, 2018
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.

None yet

7 participants