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

Move irrational constants to its own module #23427

Merged
merged 1 commit into from
Aug 31, 2017
Merged

Conversation

fredrikekre
Copy link
Member

No description provided.

log(::Irrational{:e}, x::Number) = log(x)

# align along = for nice Array printing
function alignment(io::IO, x::Irrational)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this method removed on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, should have been moved.

@Keno
Copy link
Member

Keno commented Aug 24, 2017

This was discussed in triage and the preferred resolution was the following:

  • Delete eu
  • Make (\mscre) a unicode alias for e (i.e. rebase Deprecate Euler number e in favor of ℯ (U+212F) #10612)
  • Export π and from base by default, put the ascii versions as well as the other math constants in its own module (this PR)
    Doesn't all have to be done in this PR, but I wanted to note it down somewhere relevant.

@Keno Keno added this to the 1.0 milestone Aug 24, 2017
@simonbyrne
Copy link
Contributor

I think we should probably export pi as well.

@Keno
Copy link
Member

Keno commented Aug 24, 2017

seems fine as well.

@@ -5299,7 +5299,7 @@ for (bdsdc, elty) in
u, &ldu, vt, &ldvt,
q, iq, work, iwork, info)
chklapackerror(info[])
d, e, u, vt, q, iq
d, e_, u, vt, q, iq
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently we returned that mathematical constant e from LAPACK.bdsdc!. 🤘

Copy link
Member

Choose a reason for hiding this comment

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

Just checked and e_ is just garbage after the call to xbdsdc so maybe just delete it (and adjust svd)

@fredrikekre
Copy link
Member Author

At time of writing I think this PR now implements the new behaviour. I think the easiest would be to incorporate #10612 in this PR for the deprecation of eu etc. What should we name the new module? Constants, MathConsts, MathConstants, MathematicalConstants or other suggestions?

@fredrikekre fredrikekre added deprecation This change introduces or involves a deprecation needs news A NEWS entry is required for this change labels Aug 24, 2017
@giordano
Copy link
Contributor

Note that ISO mandates that mathematical constants are to be typeset with roman (upright) font, thus using for the Euler constant is in contrast with this rule. Of course, Julia doesn't have to follow ISO on this.

@simonbyrne
Copy link
Contributor

Irrationals, since they're all irrational?

@fredrikekre
Copy link
Member Author

fredrikekre commented Aug 25, 2017

Irrationals, since they're all irrational?

In that case we should probably wrap everything regarding irrationals in that module, not just the constants.

@andyferris
Copy link
Member

I actually like that we export ASCII e (and definitely pi)

@KristofferC
Copy link
Member

KristofferC commented Aug 25, 2017

Exporting a one letter variable is way too error prone for a general purpose language, in my opinion. I have thrown e multiple times, I have had bugs in my code due to missing initializing my own \gamma etc. The cost of someone wanting to use e as Eulers constant having to do an import or using the Unicode variable seems very worthwhile if it avoids putting a one letter variable into all code.

@andyferris
Copy link
Member

Doesn't the exact same argument apply to I? (Are there other one letter ASCII exports?)

@KristofferC
Copy link
Member

It sort of does, although I guess it is more rare to use I (capital letter) in general code than e. Anyway, I think the path forward here is already decided and most the arguments for and against have been made, so perhaps not necessary to rehash them.

@SimonDanisch
Copy link
Contributor

I have never used e but use I quite a bit and definitely run into problems with it being exported ;) Guess it all depends on the naming style of a particular person!

@andyferris
Copy link
Member

andyferris commented Aug 25, 2017

Anyway, I think the path forward here is already decided and most the arguments for and against have been made, so perhaps not necessary to rehash them.

Is this referring to:

This was discussed in triage

? (edit: or is there a link to something that could have been added to the OP?) (edit2: I guess the discussion was in #10612...)

@fredrikekre
Copy link
Member Author

fredrikekre commented Aug 25, 2017

I would be the last single ASCII export after this. We could make that 𝐈 (\mbfI) or something, but thats a separate discussion.

@giordano
Copy link
Contributor

This is a breaking change, because the type of the Euler's number is different. This can probably be handled in Compat.jl by defining the same constant.

@ararslan
Copy link
Member

I must say I still don't like the Unicode alias for e. It's too visually similar to ASCII e which just makes it confusing. I think we should either keep exporting e or put it in the math constants module with no exported alias.

@fredrikekre
Copy link
Member Author

Could use some guidance on how to deprecate stuff here. Are we OK with just make this breaking? I tried some things e.g.

export e
e = Irrational{:e}()
Float64(::Irrational{:e}) = (depwarn("e is moved etc...", :Float64); Float64(ℯ))
# ...

but then you get a conflict when using Base.MathConstants:

WARNING: using MathConstants.e in module Main conflicts with an existing identifier.

Any way to actually deprecate this?

@ararslan
Copy link
Member

Should be able to use @deprecate_binding e MathConstants.e

@fredrikekre
Copy link
Member Author

Hmmmm, I was sure I tried that, but seems to work. Does not give a very helpful message though:

julia> e
WARNING: Base.e is deprecated.
  likely near no file:0= 2.7182818284590...

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Aug 28, 2017

@Keno "Make ℯ (\mscre)" What does "mscre" stand for anyway? I'm never going to remember this unless I know..

Can we have (also) \e and/or \euler for "Euler's number"?: https://en.wikipedia.org/wiki/e (mathematical constant).

Just in case it's not a good idea, note there's also: https://en.wikipedia.org/wiki/Euler–Mascheroni constant

@fredrikekre
Copy link
Member Author

\mscre = math-script-e

@Keno
Copy link
Member

Keno commented Aug 28, 2017

mscre stands for "math script e". We can and should add another unicode alias. \e has been suggested, but it's annoying, because there's a bunch of other completions that start with \e..

 - only export pi, π and ℯ from Base
 - remove eu as alias for ℯ
 - add \euler as tab completion for ℯ
@fredrikekre
Copy link
Member Author

Ok, so lets decide on a name for the module?
This is also a problem:

julia> e
WARNING: Base.e is deprecated.
  likely near no file:0= 2.7182818284590...

julia> e
WARNING: Base.e is deprecated.
  likely near no file:0= 2.7182818284590...

The warning is rather cryptic, and does not suggest the alternative and also prints more than once. This works though;

julia> using Base.MathConstants

julia> e
ℯ = 2.7182818284590...

Any ideas would be much appreciated.

@fredrikekre fredrikekre removed the needs news A NEWS entry is required for this change label Aug 28, 2017
@@ -363,6 +363,15 @@ Deprecated or removed

* `diagm(A::BitMatrix)` has been deprecated, use `diagm(vec(A))` instead ([#23373]).

* `ℯ` (written as `\mscre<TAB>` or `\euler<TAB>`) is the new default for Euler's
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean for this to be "the new default"? Default in what sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

The one being exported, and also typeof(e) has changed from Irrational{:e} to Irrational{:ℯ}.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, might be good to clarify that if you're willing!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think my own wording is kinda weird too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.