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

Add copy method for tikzdocument #321

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

rs7q5
Copy link

@rs7q5 rs7q5 commented Jun 28, 2023

Added a copy method for tikzdocument

closes PR #320.

Added a `copy` method for `tikzdocument`
@KristofferC
Copy link
Owner

KristofferC commented Jun 28, 2023

Personally, I think all our deepcopys are wrong. For example, looking at how a container in Base behaves:

julia> d1 = Dict(1 => b)
Dict{Int64, Vector{Float64}} with 1 entry:
  1 => [0.908324, 0.17025, 0.383786]

julia> d2 = copy(d1)
Dict{Int64, Vector{Float64}} with 1 entry:
  1 => [0.908324, 0.17025, 0.383786]

julia> push!(b, 1)
4-element Vector{Float64}:
 0.9083239220321686
 0.1702504972071277
 0.38378559739161744
 1.0

julia> d2
Dict{Int64, Vector{Float64}} with 1 entry:
  1 => [0.908324, 0.17025, 0.383786, 1.0]

The copy here did not recursively copy each value in the dictionary. So this type of recursive copying done by deepcopy is unexpected for data that is provided by the user.

@tpapp
Copy link
Collaborator

tpapp commented Jun 28, 2023

So this type of recursive copying done by deepcopy is unexpected for data that is provided by the user.

I am not sure what the best solution would be then.

Maybe Base.copy could

  1. copy options when applicable,

  2. for types that have an elements or contents, copy those with copy.

I think this would be closest to the intended semantics of copy.

FWIW, I use PGFPlotsX exclusively for plotting, but have never had to copy anything. So maybe I am not the best person to decide this.

@KristofferC
Copy link
Owner

Yes, I would say you want to copy the options and the elements but not recursively copy "user values".

@rs7q5
Copy link
Author

rs7q5 commented Jun 28, 2023

I will say this method did fix my error of there not being a base copy method for a function I was using that required a copy method. Granted the copy is more of a dummy copy I think. ... But yes, I usually copy the options.

Edit: should the method copy elements then but still output the td?

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.

3 participants