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

rename: MathConst => Irrational #11922

Merged
merged 1 commit into from
Jun 29, 2015
Merged

rename: MathConst => Irrational #11922

merged 1 commit into from
Jun 29, 2015

Conversation

StefanKarpinski
Copy link
Member

I have to say this feels like a much better name:

julia> typeof(π)
Irrational{}

@quinnj
Copy link
Member

quinnj commented Jun 28, 2015

+1

@tkelman
Copy link
Contributor

tkelman commented Jun 28, 2015

I like. #11200 says hello though.

@StefanKarpinski
Copy link
Member Author

Right, what we really need is a way to deprecate exported bindings. Bonus: don't autocomplete those or otherwise expose them as suggestions.


## specific mathematical constants

@math_const π 3.14159265358979323846 pi
Copy link
Contributor

Choose a reason for hiding this comment

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

Should @math_const change its name too?

@StefanKarpinski
Copy link
Member Author

Thanks for the feedback, @waldyrious!

@waldyrious
Copy link
Contributor

Glad I can help :)

@dpsanders
Copy link
Contributor

+1

=={s}(::Irrational{s}, ::Irrational{s}) = true
==(::Irrational, ::Irrational) = false

# Irationals are not rational, so unequal to everything else
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: Irationals is missing an "r". Also, I'm not sure I fully understand this comment. The beginning is redundant and the ending is kinda vague (especially since technically irrationals are reals).

StefanKarpinski added a commit that referenced this pull request Jun 29, 2015
rename: MathConst => Irrational
@StefanKarpinski StefanKarpinski merged commit 5444813 into master Jun 29, 2015
@StefanKarpinski StefanKarpinski deleted the sk/irrational branch June 29, 2015 02:04
@goszlanyi
Copy link

-1
Just an opinion: MathConst was a better name.

@simonbyrne
Copy link
Contributor

Now we have generated functions, we could also get rid of the numeric val argument to the macro: since we're renaming, perhaps this would be a good time to make the switch?

@simonbyrne
Copy link
Contributor

Also, I like the change: it helps make clear why pi != float(pi)

@ivarne
Copy link
Member

ivarne commented Jun 29, 2015

I like this change, but 5 hours seems like a too short comment period, especially with a global audience.

waldyrious added a commit to waldyrious/julia that referenced this pull request Jun 29, 2015
@StefanKarpinski
Copy link
Member Author

Yeah, sorry about that. Got a bit trigger happy.

@simleb
Copy link

simleb commented Jun 29, 2015

I'm not convinced. It might look nicer to the eye but then why wouldn't π+1 be promoted to Irrational as well? What about √2?

What I'm saying is that using the word irrational, which has a well understood mathematical meaning, for a type that wraps mathematical constants which happen to be irrational but are just a tiny subset of all irrational numbers, especially when the language already has a Rational type with a widely different use (integers are promoted to rational for instance…), is confusing.

I know it's just a name, and the type of these mathematical constants is meant to be transient (in the sense that they almost always appear as part of expressions which convert them) but it is confusing I think.

tl;dr What is Irrational? The type used for mathematical constants… obviously!

@simonbyrne
Copy link
Contributor

@simleb Good point. What about IrrationalConst?

@dpsanders
Copy link
Contributor

+1 for IrrationalConst. (I was just typing almost the same.)

@ScottPJones
Copy link
Contributor

👍 also for IrrationalConst

@goszlanyi
Copy link

We had a good, obvious and short name for this.
Why not leave it as is?

@StefanKarpinski
Copy link
Member Author

MathConst was far from obvious – there was a fair amount of confusion about it over time and it always bothered me. To argue in favor of Irrational rather than IrrationalConst, this type is capable of representing arbitrary irrational values as long as you have a way to define them. It's not like anyone is going to come up with a clever encoding that can represent all irrational numbers.

@waldyrious
Copy link
Contributor

I think IrrationalConst may be clearer since (in computing) using the term "constants" specifies that these are hand-picked numbers rather than a type meant to represent any irrational number.


big(x::Irrational) = convert(BigFloat,x)

## specific irriational mathematical constants

Choose a reason for hiding this comment

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

typo here: "irriational" -> "irrational".

@simleb
Copy link

simleb commented Jun 29, 2015

Well, the name Irrational is starting to grow on me. I agree with the arguments set forth by Stefan in #11929: Irrational simply couldn't mean anything else in the context of Julia. There is no way to even test if a number is irrational so there is no way to create them outside of how it is done now: by providing an algorithm to compute them to arbitrary precision. And since these numbers clearly have to be constant, there is no need for Const in the name. (Plus there is really no advantage using IrrationalConst over MathConst.)

Thus from now on I fully support the name Irrational for these mathematical constants (until someone writes a clever rebuttal, haha)!

KristofferC pushed a commit to KristofferC/julia that referenced this pull request Jun 30, 2015
simonster added a commit to JuliaLang/Compat.jl that referenced this pull request Jun 30, 2015
simonster added a commit to JuliaLang/Compat.jl that referenced this pull request Jun 30, 2015
simonster added a commit to JuliaLang/Compat.jl that referenced this pull request Jul 1, 2015
Aerlinger added a commit to JuliaMath/Tau.jl that referenced this pull request Dec 26, 2016
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.