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

Transfer ArrayViews from package to Base #8176

Closed
wants to merge 2 commits into from
Closed

Conversation

andreasnoack
Copy link
Member

This is an updated version of #5556. I have copied the content of ArrayViews.jl to Base and committed with @lindahua as author. Are you okay with this @lindahua?

@IainNZ
Copy link
Member

IainNZ commented Aug 29, 2014

Factoid: ArrayViews.jl is required directly or indirectly by 75 other packages:

Top 10 packages by number of packages that depend on them:
URIParser          85
SHA                84
BinDeps            83
ArrayViews         75
JSON               66
StatsBase          65
Homebrew           54
Zlib               46
URLParse           39
Reexport           38

@lindahua
Copy link
Contributor

I have no problem with this.

@jakebolewski
Copy link
Member

Poking around with this, when building the system image we need to provide convert functions for ArrayViews -> Arrays. However this gives a MethodError as dispatch is calling jl_f_convert_default instead of the convert methods defined in arrayviews.jl. I'm still trying to wrap my head around the system image build process, @vtjnash any ideas?


getindex(a::ArrayView) = getindex(a.arr, a.offset + 1)

getindex(a::ArrayView, i::Int) = getindex(a.arr, uindex(a, i))
Copy link
Member

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)

@vtjnash
Copy link
Member

vtjnash commented Aug 30, 2014

@jakebolewski either you've managed to cache an old version of convert, or perhaps have typed it too strongly, so it didn't apply when it should have. i can't say too much without seeing the code

@staticfloat
Copy link
Member

@IainNZ I love that since SHA snuck in under BinDeps it immediately rocketed to the top. XD

@jakebolewski
Copy link
Member

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.

@vtjnash
Copy link
Member

vtjnash commented Aug 30, 2014

much too strongly it looks like. almost all of those methods should be of the form:
fname{A<:Array}(::Type{A}, ...)
to avoid forcing requirements on the presence / value of type parameters

@andreasnoack andreasnoack force-pushed the anj/arrayviews branch 3 times, most recently from 1e4a891 to 0d4a996 Compare September 3, 2014 18:18
@andreasnoack
Copy link
Member Author

For some reason this keeps failing on Travis, but the tests have passed on four machines locally. This PR just adds the view function and the Comtiguous- and StridedViews. It doesn't change the behavior of getindex or removes SubArrays which I think are the next steps in the transition. Hence I think we can merge this PR very soon.

@StefanKarpinski
Copy link
Member

+1 — I would say go for it and see if anyone yells.

@IainNZ
Copy link
Member

IainNZ commented Sep 3, 2014

Our Travis tests have been failing badly, I wouldn't put much stock in it.

@staticfloat
Copy link
Member

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.

@quinnj
Copy link
Member

quinnj commented Sep 3, 2014

How long are the arrayviews tests supposed to take? Not sure if mine have hung or what, but it's still running after a few minutes........

windows 8.1 64-bit.

@timholy
Copy link
Member

timholy commented Sep 3, 2014

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 Arrays, and there's a whole world of AbstractArrays that are only handled by SubArray (and ArrayViewsAPL). I, for one, use views of AbstractArrays about 100x more frequently than views of plain Arrays.

See also #7941 (comment), and my followup below that.

@quinnj
Copy link
Member

quinnj commented Sep 3, 2014

Not sure if it was causing the hang, but I needed to put using Base.ArrayViews and import Base.ArrayViews to get the tests to pass for me locally.

@andreasnoack
Copy link
Member Author

@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 SubArray. Would the following make sense?

  • Merge this PR
  • Open a new PR experimenting with changing getindex to use views for Arrays
  • Leave SubArrays until staged functions land and then start substituting SubArrays with functionality from ArrayViewsAPL

The last step could possibly also change Contiguous- and StridedViews if necessary. I think the important thing is that people start to get used to get views when indexing arrays.

@andreasnoack
Copy link
Member Author

@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?

@timholy
Copy link
Member

timholy commented Sep 4, 2014

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.

@quinnj
Copy link
Member

quinnj commented Nov 22, 2014

Close since #8501 was merged?

@lindahua lindahua closed this Nov 22, 2014
@andreasnoack andreasnoack deleted the anj/arrayviews branch March 24, 2017 14:23
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.

9 participants