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

Fix typo & attempt at more explicit comment #11929

Merged
merged 1 commit into from
Jun 29, 2015

Conversation

waldyrious
Copy link
Contributor

@waldyrious waldyrious commented Jun 29, 2015

Originally commented here: #11922 (comment)

@tkelman
Copy link
Contributor

tkelman commented Jun 29, 2015

Devils' advocate: are all Real types guaranteed to have a finite representation?

@waldyrious
Copy link
Contributor Author

Well technically the "irrational" constants in julia aren't the real deal, since they have to be defined somewhere, and that definition is of course a finite approximation. In that sense, technically a MathConst (well, Irrational) should be equal to the representation that originated it, e.g. π == 3.14159265358979323846 should return true, not false as the lines below this comment force. I interpret the implemented behavior more as a theoretical result than a technical one, and this comment somehow goes along with that. It might not be clear, though, so any suggestions for improvement are welcome :)

@StefanKarpinski
Copy link
Member

Devils' advocate: are all Real types guaranteed to have a finite representation?

It's a fair point but until such types are added, it's correct to say that an irrational and another non-irrational real are guaranteed not to be equal. We could add AbstractRational and AbstractIrrational subtypes of Real but that seems like too much hierarchy.

StefanKarpinski added a commit that referenced this pull request Jun 29, 2015
Fix typo & attempt at more explicit comment
@StefanKarpinski StefanKarpinski merged commit 1544e4e into JuliaLang:master Jun 29, 2015
@waldyrious waldyrious deleted the patch-1 branch June 29, 2015 15:31
@ScottPJones
Copy link
Contributor

Could this be better expressed as a trait of a Real number?

@dpsanders
Copy link
Contributor

a MathConst (well, Irrational) should be equal to the representation that originated it, e.g. π == 3.14159265358979323846

I don't agree. The point of having IrrationalConst is that it allows us to do more nuanced things, e.g.:

julia> Float64(pi, RoundDown)
3.141592653589793

julia> Float64(pi, RoundUp)
3.1415926535897936

This shows exactly that pi is a true real number that exists somewhere between these two adjacent floating-point numbers. Also,

julia> big(pi)
3.141592653589793238462643383279502884197169399375105820974944592307816406286198

So the situation is much more subtle than just saying that pi is equal to 3.14159265358979323846

@StefanKarpinski
Copy link
Member

Well technically the "irrational" constants in julia aren't the real deal, since they have to be defined somewhere, and that definition is of course a finite approximation.

This isn't really true. There's a definition for each Irrational of how to compute it as a BigFloat, which can be done to arbitrary precision. Thus an algorithm is provided for each Irrational that can compute it to as many digits as one cares to have.

@waldyrious
Copy link
Contributor Author

Thanks for the clarifications. I didn't (and to be honest don't fully yet) understand the role of val in @irrational. Is it meant as a quick lookup table in case a float is being requested, to bypass the need to call MPFR? Maybe adding some documentation to the macro or to the lines where the macro is called would be helpful.

In any case, I'm happy to stand corrected if there's a better way to phrase this comment.

@StefanKarpinski
Copy link
Member

Is it meant as a quick lookup table in case a float is being requested, to bypass the need to call MPFR?

It's to avoid bootstrapping issues. At the point where this file is loaded, BigFloat hasn't been loaded yet so we can't call MPFR to compute the Float64 value.

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.

5 participants