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

Remove collect #661

Closed
wants to merge 4 commits into from
Closed

Remove collect #661

wants to merge 4 commits into from

Conversation

sklan
Copy link
Contributor

@sklan sklan commented Mar 5, 2019

Solves #657

julia> xs = param([1, 2, 3])
Tracked 3-element Array{Float64,1}:
 1.0
 2.0
 3.0

julia> collect(xs)
3-element Array{Flux.Tracker.TrackedReal{Float64},1}:
 1.0
 2.0
 3.0

Solves #657
@MikeInnes
Copy link
Member

Can you add a definition for collect that just returns x? Keeping this as a TrackedArray is more efficient.

@sklan
Copy link
Contributor Author

sklan commented Mar 5, 2019

Yeah, sure. I'll do that.

@sklan
Copy link
Contributor Author

sklan commented Mar 6, 2019

@MikeInnes It still keeps calling the Base.collect

julia> xs = param([1, 2, 3])
Tracked 3-element Array{Float64,1}:
 1.0
 2.0
 3.0

julia> collect(xs)
3-element Array{Flux.Tracker.TrackedReal{Float64},1}:
 1.0
 2.0
 3.0

julia> @which collect(xs)
collect(A::AbstractArray) in Base at array.jl:546

I am confused. Am I missing something?

@MikeInnes
Copy link
Member

You need to define Base.collect (otherwise it creates a new function, Flux.collect).

src/tracker/lib/array.jl Outdated Show resolved Hide resolved
@MikeInnes
Copy link
Member

Unfortunately this got caught when we split out Tracker.jl. You should be able to easily PR the same branch against that repo, though.

@sklan
Copy link
Contributor Author

sklan commented Apr 4, 2019

Okay, I'll add a PR in Tracker.jl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants