QuantEcon / QuantEcon.jl

Julia implementation of QuantEcon routines
https://quantecon.org/quantecon-jl/
BSD 3-Clause "New" or "Revised" License
506 stars 303 forks source link

Ranges preparation for v1.1 for replacing linspace #220

Closed jlperla closed 6 years ago

jlperla commented 6 years ago

The current range syntax changing between 1.0 and 1.1... It may only be a few months before the 1.1 release, but we are writing a lot of code between now and then.

Basically, whenever we have code right now that says linspace(a, b, N) with this patch we could turn it to range(a, b, length=N) which is objectively better.

I asked on slack and they told me that if we do some (temporary) type piracy, we can emulate the (far better) syntax with

Base.range(start, stop; length=nothing, step=nothing) = range(start; stop=stop, length=length, step=step)

We would wrap that in a v"1.0" <= VERSION < v"1.1" wrapper so as not to clash with the 1.1 release.

@sglyon @Nosferican @cc7768 Thoughts?

albop commented 6 years ago

I'm curious : how is range () objectively better than linspace ?

Le ven. 31 août 2018 à 19:47, Jesse Perla notifications@github.com a écrit :

The current range syntax changing between 1.0 and 1.1... It may only be a few months before the 1.1 release, but we are writing a lot of code between now and then.

Basically, whenever we have code right now that says linspace(a, b, N) with this patch we could turn it to range(a, b, length=N) which is objectively better.

I asked on slack and they told me that if we do some (temporary) type piracy, we can emulate the (far better) syntax with

Base.range(start, stop; length=nothing, step=nothing) = range(start; stop=stop, length=length, step=step)

We would wrap that in a v"1.0" <= VERSION < v"1.1" wrapper so as not to clash with the 1.1 release.

@sglyon https://github.com/sglyon @Nosferican https://github.com/Nosferican @cc7768 https://github.com/cc7768 Thoughts?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/QuantEcon/QuantEcon.jl/issues/220, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ5KRKpoMg6qFItknOx4Po4p8i8q1pHks5uWXbBgaJpZM4WVnZF .

jlperla commented 6 years ago

I suppose that is a matter of taste, but I like forced keyword arguments when things are unclear of the ordering.

Regardless, it is what Julia is doing. What is worse than linspace is the current range in Julia v1.0 - which this is intended to temporarily patch.

sglyon commented 6 years ago

my gut reaction is that it scares me a bit to define that method...

Are you trying to expose this method to users (not devs) who call using QuantEcon?

jlperla commented 6 years ago

It should be scary, it is unequivocable type piracy!

Yes, the idea would be that the julia lecture notes (and anyone who does using QuantEcon) would be able to use the new syntax. Once Julia 1.1 is released, though, that function would not exported or even defined as https://github.com/JuliaLang/julia/pull/28708 would be merged. This is one of those PRs that should have made it into v1.0 but missed the window.

The code listed above comes from a Julia core dev (@simonbyrne ) who suggested that we could just add in the method from the PR temporarily if we chose to.

simonbyrne commented 6 years ago

If only want to use it within the package, one way to avoid type piracy is to add your own range function (which could later be deleted). This is more or less what Compat does.

range(start; stop=nothing, length=nothing, step=nothing) = 
    Base.range(start; stop=stop, length=length, step=step)
range(start, stop; length=nothing, step=nothing) = 
    Base.range(start; stop=stop, length=length, step=step)
simonbyrne commented 6 years ago

The code listed above comes from a Julia core dev (@simonbyrne ) who suggested that we could just add in the method from the PR temporarily if we chose to.

I would definitely not condone type piracy in a package (what you do in your own code is up to you)

jlperla commented 6 years ago

@simonbyrne I just want people to be able to use the new notation if they are using QuantEcon, as we have so much code that relies on linspace, and the v1.0 syntax which got rid of linspace is so awful.

range(start; stop=nothing, length=nothing, step=nothing) = 
    Base.range(start; stop=stop, length=length, step=step)
range(start, stop; length=nothing, step=nothing) = 
    Base.range(start; stop=stop, length=length, step=step)

The problem is that due to julia's inability to merge methods effectively it would mean people need to use namespace qualification for the range, unless I am missing something. That defeats the purpose of just trying to patch code until v1.1 is out.

Type piracy is always something to be extremely cautious with, but what is the particular worry about it in this exact circumstance? What exactly would go wrong?

simonbyrne commented 6 years ago

Because random packages shouldn't change the behaviour of unrelated Base functionality.

Yes, I agree since it's a method error, it's not that big of a deal, but you could still have a situation where someone has some code which uses the this functionality, which then breaks when they forget using QuantEcon. Hunting down that bug would be very confusing.

If it should go anywhere, it should be in Compat.jl.

jlperla commented 6 years ago

Because random packages shouldn't change the behaviour of unrelated Base functionality.

True almost always. But in this exact circumstance, it is about temporarily accelerating a feature that will be in Base, is currently a method error, and isn't exported when base changes. It may seem trivial, but this is such a common programming pattern that I care deeply about making sure people use the right (i.e. 2-3 months from now) way of doing it.

Yes, I agree since it's a method error, it's not that big of a deal, but you could still have a situation where someone has some code which uses the this functionality, which then breaks when they forget using QuantEcon.

If that is the worst thing that could happen, I think that is OK. And that would only be for the next 2-3 months (hopefully), at which point the same code would work on base.

If it should go anywhere, it should be in Compat.jl.

I don't believe that most user code uses Compat.jl? I never do, at least? Or am I missing something there.

Nosferican commented 6 years ago

I would use the syntax based on the language. We can release a new version for v"1.1.z". If there are issues I think Compat would be the place if anything and it would probably be there. We could add it as a dependency even if not my favorite solution.

jlperla commented 6 years ago

I also want to point out that this is a sufficiently special case that people in https://github.com/JuliaLang/julia/pull/28708 were even arguing about breaking SemVer to get it in.

I would use the syntax based on the language. We can release a new version for v"1.1.z".

A lot of code would be created and taught prior to that point, using a confusing pattern that everyone agrees will be changed in the short term. The language was released before it was ready, so some special cases may be acceptable to help in patching it. I don't think this sets a precedent.

I think that telling users to setup Compat.jl as a dependency on user code is a bad idea.

Nosferican commented 6 years ago

If the course is started with Julia 1.y.z. best practice would be to not upgrade a major version for that term so students would most likely not see the new syntax until after the course.

jlperla commented 6 years ago

Students (at least mine) will start with 1.0.

Who knows when v1.1 will actually be released? You are also assuming that students will notice and change the way they code between versions... these things are a mental snapshot for most people. They will take whatever source we give them and use it directly for the future unless something forces them to change it. This linspace debacle will be the most visible (and only known?) change in the code between the two versions that I can see.

You guys can outvote me, but this seems a reasonable special case to be made.

Nosferican commented 6 years ago

I started learning Python soon after 3.y.z got released and learned the language in three different classes using 2.7. It was a bummer trying to learn the changes, but it wasn't too bad and I think Julia is making it a lot easier than what it was for Python at that historic time. Say that we teach them the 1.1 syntax, who will "shelter" them from when 1.2 comes out? I would be surprised if v"1.1.0" does not allow the previous syntax and just gives a deprecation warning (will it not deprecate the syntax?).

jlperla commented 6 years ago

Say that we teach them the 1.1 syntax, who will "shelter" them from when 1.2 comes out?

No precedent is intended on any other features or versions. I expect v1.2 to be a normal point release and staggered with a normal timeline. Whether people admit it or not, v1.1 is very likely going to be a fix to the v1.0 release getting in whatever they should have snuck in to v1.0 but couldn't because of the hard deadline on the release date.

I also don't believe that v1.1 is supposed to deprecate the current syntax. Would that change anything?

sglyon commented 6 years ago

I don't believe that most user code uses Compat.jl? I never do, at least? Or am I missing something there.

won't matter because as long as some package they load uses compat, the method will be added to the Base.range function

jlperla commented 6 years ago

won't matter because as long as some package they load uses compat, the method will be added to the Base.range function

My concern was that I would prefer not to make Compat.jl a dependency for lectures.

Your concern seems to be that when this code is added to Compat.jl (which it will be there at some point soon, I would guess) that it would clash with any QuantEcon.jl version of the function?

sglyon commented 6 years ago

Nope, I was addressing your concern about not loading Compat explicitly in script type code.

You would have the method as long as somewhere in the stack (e.g. in QuantEcon) a package loads Compat. As soon as that happens the method is added to the range function and available globally.

If we wanted to find a home for it and ensure that it is ready for QuantEcon lecture users we should put it in Compat and call using Compat in QuantEcon so that as soon as somebody does using QuantEcon the method is guaranteed to be added.

Same "lecture user" effect as defining it here ourselves, but it can be shared across all of Julia and Compat gets to be the pirate, not us

jlperla commented 6 years ago

I think I see what you are saying for the clashes between this and one added into compat. That kills the idea.

So... changing approaches, but not my end goal :-)

If we tell people to add using Compat in the lecture notes, which is not an enormous problem, and could help us sneak in other v1.0 to v1.1 quirks, is there any real downside? Can it break anything, slow things down in compiled code, etc.?

sglyon commented 6 years ago

I don't think I was clear enough

Here's the idea:

jlperla commented 6 years ago

I see. Are there any downsides to that approach? Are there times when you really don't want to have Compat.jl?

sglyon commented 6 years ago

Not that I know.

The downside will be convincing Compat stewards to accept a "forward compatibility" method instead of the typical backward compatibility-preserving definitions you usually find there

albop commented 6 years ago

Maybe they would be open to "from future import range" ? ;-)

Joke appart, I'm fairly neutral on this issue as it's really a transitional problem. Compat sounds like a good idea.

What the students should know is that keyword arguments are now fast with Julia so there is no need to find strategies to avoid them. range is a good example of that. Now I'll be teaching Julia again soon and I sincerely hope 1.x will be stable in the future, but maybe we should tell students snapshoting a mental representation of Julia wasn't a winning strategy in the past...

On Fri, Aug 31, 2018 at 10:48 PM Spencer Lyon notifications@github.com wrote:

Not that I know.

The downside will be convincing Compat stewards to accept a "forward compatibility" method instead of the typical backward compatibility-preserving definitions you usually find there

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/QuantEcon/QuantEcon.jl/issues/220#issuecomment-417785259, or mute the thread https://github.com/notifications/unsubscribe-auth/AAQ5KQEqAcSs3_bZrob0CRr39Ldv9njpks5uWaErgaJpZM4WVnZF .

jlperla commented 6 years ago

but maybe we should tell students snapshoting a mental representation of Julia wasn't a winning strategy in the past...

That is for sure. The problem is that it is hard for them to acquire the new information - unless errors are triggered, which wouldn't occur here. Also a great deal of hysterisis occurs with code.

It looks like from https://github.com/JuliaLang/julia/pull/28708#issuecomment-418433463 that adding to Compat.jl would have support, so I will close this issue and we can do something separate for the Compat.jl.