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 duplicates arrays #13124

Closed
denizyuret opened this issue Sep 14, 2015 · 8 comments
Closed

deepcopy duplicates arrays #13124

denizyuret opened this issue Sep 14, 2015 · 8 comments
Labels
breaking This change will break code

Comments

@denizyuret
Copy link
Contributor

a=rand(3,5)
b=(a,a)
b[1]===b[2] => true
c=deepcopy(b)
c[1]===c[2] => false

To fix this issue, on line 59 of deepcopy.jl we can replace:

        return copy(x)

with

        return (stackdict[x]=copy(x))
@StefanKarpinski
Copy link
Member

This does seem to be a consistent change with the premise that deepcopy should be equivalent to serialize and deserialize.

@simonster simonster added the breaking This change will break code label Sep 14, 2015
@JeffBezanson
Copy link
Member

Agreed, though currently the serializer does not accurately record shared references to bits arrays. I found it impossible to get acceptable performance with strings when doing that.

@StefanKarpinski
Copy link
Member

Could we special case strings since they're mutable in implementation but immutable in usage?

@timholy
Copy link
Member

timholy commented Sep 26, 2015

I use deepcopy in Images precisely because it copies arrays: JuliaImages/Images.jl#356 (comment). I guess I've always thought that deepcopy's purpose is to give me a totally-independent version of something, so that if I mutate it the original will be unchanged.

Should Dicts instead make copies of all their keys and values? Or should I write a custom copy function?

@StefanKarpinski
Copy link
Member

Would your use-case really be messed up by not copying strings? That seems pretty benign to me since one shouldn't mutate the storage of strings.

@timholy
Copy link
Member

timholy commented Sep 26, 2015

It's more the "value" than the "key" I'm worried about. E.g., "pixelspacing" => [2mm, 2mm] you don't want to change the original if you set img["pixelspacing"][2] = 3mm.

@denizyuret
Copy link
Contributor Author

I am a bit confused: I was not proposing any sharing between the original
and its deepcopy. It is just that if two things are shared in the source,
they should be independently created but in a shared way in the deepcopy.
For example if a=rand(3,4) and b=(a,a), when we do c=deepcopy(b), there is
no sharing between b and c, just between c[1] and c[2].

best,
deniz

On Sat, Sep 26, 2015 at 4:09 PM Tim Holy [email protected] wrote:

It's more the "value" than the "key" I'm worried about. E.g., "pixelspacing"
=> [2mm, 2mm] you don't want to change the original if you set img["pixelspacing"][2]
= 3mm.


Reply to this email directly or view it on GitHub
#13124 (comment).

@timholy
Copy link
Member

timholy commented Sep 27, 2015

I didn't read the test case carefully enough. Yes, that's a problem. Carry on 😄.

bjarthur pushed a commit to bjarthur/julia that referenced this issue Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This change will break code
Projects
None yet
Development

No branches or pull requests

5 participants