-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Transfer ArrayViews from package to Base #8176
Conversation
Factoid:
|
I have no problem with this. |
Poking around with this, when building the system image we need to provide |
|
||
getindex(a::ArrayView) = getindex(a.arr, a.offset + 1) | ||
|
||
getindex(a::ArrayView, i::Int) = getindex(a.arr, uindex(a, i)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these methods (since abstract), should generally not be assuming that a.arr
is valid, but instead should call parent(a)
@jakebolewski either you've managed to cache an old version of |
@IainNZ I love that since |
Typing it too strongly was my guess. Are you referencing the only line in inference.jl where you throw a method error? That doesn't seem to be the case but I could be wrong about that. I've pushed the current state to https://github.com/JuliaLang/julia/compare/jcb/arrayview branch, it should fail in file.jl. |
much too strongly it looks like. almost all of those methods should be of the form: |
1e4a891
to
0d4a996
Compare
For some reason this keeps failing on Travis, but the tests have passed on four machines locally. This PR just adds the |
+1 — I would say go for it and see if anyone yells. |
Our Travis tests have been failing badly, I wouldn't put much stock in it. |
Your linux failure is #8200 (I can type that issue number by heart now). Your mac failure looks like travis just gave up while waiting for output, but it made its way through an awful lot of tests, so I'd say go for it. |
How long are the windows 8.1 64-bit. |
I love the idea of merging ArrayViews, but you cannot remove SubArrays until there's a plan to more forward more generally---ArrayViews wrap only See also #7941 (comment), and my followup below that. |
Not sure if it was causing the hang, but I needed to put |
@timholy I see. My priority was to push the new behavior for array indexing to master as soon as possible such that everyone can start adjusting their code. I hadn't really grasped the problem of removing
The last step could possibly also change |
0d4a996
to
62d9d6e
Compare
@quinnj I think you are experiencing the same error as I saw on Travis. Could you try to checkout the latest version and run the tests again? |
I'm fine with that plan. But FYI, what I'm working on ATM is generating a version of ArrayViewsAPL that doesn't rely on stagedfunctions. It's quite possible I'll have a preliminary version working by tomorrow. You could either go ahead with your plan, or wait and see what that looks like before making a decision. |
2ef98c5
to
0388647
Compare
Close since #8501 was merged? |
This is an updated version of #5556. I have copied the content of
ArrayViews.jl
toBase
and committed with @lindahua as author. Are you okay with this @lindahua?