Navigation Menu

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

Ranges preparation for v1.1 for replacing linspace #220

Closed
jlperla opened this issue Aug 31, 2018 · 24 comments
Closed

Ranges preparation for v1.1 for replacing linspace #220

jlperla opened this issue Aug 31, 2018 · 24 comments

Comments

@jlperla
Copy link
Member

jlperla commented Aug 31, 2018

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
Copy link

albop commented Aug 31, 2018 via email

@jlperla
Copy link
Member Author

jlperla commented Aug 31, 2018

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
Copy link
Member

sglyon commented Aug 31, 2018

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
Copy link
Member Author

jlperla commented Aug 31, 2018

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 JuliaLang/julia#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
Copy link

simonbyrne commented Aug 31, 2018

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
Copy link

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
Copy link
Member Author

jlperla commented Aug 31, 2018

@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
Copy link

simonbyrne commented Aug 31, 2018

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
Copy link
Member Author

jlperla commented Aug 31, 2018

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
Copy link
Contributor

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
Copy link
Member Author

jlperla commented Aug 31, 2018

I also want to point out that this is a sufficiently special case that people in JuliaLang/julia#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
Copy link
Contributor

Nosferican commented Aug 31, 2018

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
Copy link
Member Author

jlperla commented Aug 31, 2018

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
Copy link
Contributor

Nosferican commented Aug 31, 2018

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
Copy link
Member Author

jlperla commented Aug 31, 2018

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
Copy link
Member

sglyon commented Aug 31, 2018

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
Copy link
Member Author

jlperla commented Aug 31, 2018

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
Copy link
Member

sglyon commented Aug 31, 2018

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
Copy link
Member Author

jlperla commented Aug 31, 2018

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
Copy link
Member

sglyon commented Aug 31, 2018

I don't think I was clear enough

Here's the idea:

  • Add your method to Compat.jl
  • Then call using Compat in QuantEcon.jl (the library). This causes the new method to be added to the function Base.range and available anywhere Base.range is available
  • Lecture users call using QuantEcon. QuantEcon then loads compat and without the lecture user even needing to think about it, the new method (that lives in Compat) is available for them to use

@jlperla
Copy link
Member Author

jlperla commented Aug 31, 2018

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

@sglyon
Copy link
Member

sglyon commented Aug 31, 2018

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
Copy link

albop commented Sep 1, 2018 via email

@jlperla
Copy link
Member Author

jlperla commented Sep 4, 2018

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
JuliaLang/julia#28708 (comment) that adding to Compat.jl would have support, so I will close this issue and we can do something separate for the Compat.jl.

@jlperla jlperla closed this as completed Sep 4, 2018
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

No branches or pull requests

5 participants