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 the TArray related docs #124

Closed
rikhuijzer opened this issue Feb 20, 2022 · 6 comments · Fixed by #152
Closed

Remove the TArray related docs #124

rikhuijzer opened this issue Feb 20, 2022 · 6 comments · Fixed by #152

Comments

@rikhuijzer
Copy link
Contributor

From a comments by @yebai in TuringLang/Turing.jl#1778 (comment):

By the way, soon we can also remove the TArray related docs since the new Libtask can deep-copy heap-allocated objects - we still need to activate it. See

# ?? should we deepcopy Array and Dict by default?

@yebai
Copy link
Member

yebai commented Mar 8, 2022

After #129 is complete, we can depreciate all T* data structures, and create a switch for enabling _binding_copy for all Julia types.

@yebai
Copy link
Member

yebai commented Jun 15, 2022

@rikhuijzer would you like to give it another go to depreciate all the TArray and TRef data structures? The new tape_copy API now allows us to copy objects of any user-specified types.

@rikhuijzer
Copy link
Contributor Author

@yebai I won't be able to pick that up in the near future unfortunately

@yebai
Copy link
Member

yebai commented Jun 19, 2022

@KDr2 let's create a PR to depreciate all the TArray / TRef APIs, and suggest to the user to use Array and Ref as a replacement. Then we can switch on deep-copying behaviour for Array and Ref types by default.

@yebai
Copy link
Member

yebai commented Jun 19, 2022

Then we can switch on deep-copying behavior for Array and Ref types by default.

For backward compatibility, e.g. this example in README, maybe we can consider a keyword argument to the TapedTask constructor, i.e.

# Default option: when `deepcopy` is empty, then we don't perform deep copy for any types
TapedTask(...; deepcopy=[])

# Switch on deep copy behavior for Array, Dict, Ref
#  Need to update AdvancedPS to explicitly specify `deepcopy` 
TapedTask(...; deepcopy=[Array, Dict, Ref])


# In order to implement `tape_copy` for this design, we might need to 
#  dispatch on `TapedTask` instances, e.g. 

tape_copy(Val{tapedtask}, ...)

@KDr2
Copy link
Member

KDr2 commented Jun 19, 2022

a keyword argument to the TapedTask constructor

If we do this, we should add the same argument to TapedFunction and tape_copy? if so, it's not very clean I think.

tape_copy(Val{tapedtask}, ...)

I don't understand this, now we only tape_copy values in bindings.

@KDr2 KDr2 linked a pull request Jun 22, 2022 that will close this issue
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 a pull request may close this issue.

3 participants