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

Deepcopy of Dictionaries in combination with mutating leads to errors. #1233

Open
ntausend opened this issue Jun 1, 2022 · 3 comments
Open
Labels
bug Something isn't working dictionary help wanted Extra attention is needed

Comments

@ntausend
Copy link

ntausend commented Jun 1, 2022

Hey, I discovered a strange behavior which I am not sure it is intended in that way.

Consider the following example code:

function mutating1(d, a)
	upd = d[1] * a
	d[1] = upd
	return d
end

function mutating2(d,a)
	nd = deepcopy(d)
		
	upd = d[1] * a
	nd[1] = upd
	return nd
end

function mutating3(d,a)
	nd = deepcopy(d)
		
	upd = nd[1] * a
	d[1] = upd
	return d
end

function nrmsq(d)
	res = norm(d[1]*d[2])
	return res
end

d = 3
arry = map(_ -> randn(d,d), 1:2)
dic = Dict{Any, Any}()
for (jj, tm) in enumerate(arry)
    dic[jj] = tm
end

dicco = deepcopy(dic)
f1 = x -> nrmsq(mutating1(x, 5))
f2 = x -> nrmsq(mutating2(x, 5))
f3 = x -> nrmsq(mutating3(x, 5))

# all three are equal, make sure f1 which modifies the input is called last
# same holds for f3, so make sure to call it on an copy.
f2(dic) |> display
f3(dicco) |> display
f1(dic) |> display

# works
g1 = gradient(f1, dic)[1]

# do not work
g2 = gradient(f2, dic)[1]
g3 =gradient(f3, dic)[1]

The versions utilizing f2 and f3 are leading to strange AssertionError: a === b from accum while the version with f1 works flawless.

Actually I have no idea how to deal with this and how to get an workaround.

Thanks for help in advance!

@ToucheSir ToucheSir added bug Something isn't working help wanted Extra attention is needed dictionary labels Jun 3, 2022
@ToucheSir
Copy link
Member

The assertion in question: https://github.com/FluxML/Zygote.jl/blob/v0.6.40/src/lib/base.jl#L27. I don't see any reason to not support this form of mutation as well, but perhaps there are some edge cases I've not thought of.

@ntausend
Copy link
Author

ntausend commented Jun 4, 2022

Ok thanks for the reply.

Yes i guess thats the raised assertion, but I do not understand why its raised since I also not understand how mutation is implemented.

I tried understanding the grad_mut(__contex__) appearing in all functions.

So if its possible for you to including this case it would be great, otherwise I will find an alternative to that problem :)

@CarloLucibello
Copy link
Member

Current status of this is that the gradient doesn't throw an error anymore but g2 and g3 are different from g1:

julia> g1
Dict{Any, Any} with 2 entries:
  2 => [25.294 5.80564 -3.12013; 1.44079 -11.202 0.970101; -12.9575 7.81748 -0.0808782]
  1 => [3.31527 0.25157 -3.46752; 5.09686 -1.24741 -5.60557; -0.426827 -0.741984 0.333482]

julia> g2
Dict{Any, Any} with 2 entries:
  2 => [126.47 29.0282 -15.6007; 7.20393 -56.0099 4.8505; -64.7873 39.0874 -0.404391]
  1 => [3.31527 0.25157 -3.46752; 5.09686 -1.24741 -5.60557; -0.426827 -0.741984 0.333482]

julia> g3
Dict{Any, Any} with 2 entries:
  2 => [126.47 29.0282 -15.6007; 7.20393 -56.0099 4.8505; -64.7873 39.0874 -0.404391]
  1 => [3.31527 0.25157 -3.46752; 5.09686 -1.24741 -5.60557; -0.426827 -0.741984 0.333482]

julia> g2 == g3
true

julia> g1 == g2
false

julia> g1[1]  g2[1]
true

julia> g1[2]  g2[2]
false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dictionary help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants