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

Implement __copy__ and __deepcopy__ model methods #6985

Closed
Tracked by #7053
ricardoV94 opened this issue Nov 2, 2023 · 5 comments · Fixed by #7492
Closed
Tracked by #7053

Implement __copy__ and __deepcopy__ model methods #6985

ricardoV94 opened this issue Nov 2, 2023 · 5 comments · Fixed by #7492

Comments

@ricardoV94
Copy link
Member

ricardoV94 commented Nov 2, 2023

Description

A couple of users on discourse got some nasty bugs when trying to do copy.deepcopy(model). Whatever the default is doing, it ain't great.

We have a clone_model in model/fgraph that we can use for both copy and deepcopy!

It should work in all cases except for the known issue with GP variables #6883

@ricardoV94 ricardoV94 changed the title Implement __copy__ and __deepcopy__ model methods Implement __copy__ and __deepcopy__ model methods Nov 2, 2023
@Dekermanjian
Copy link
Contributor

Hey @ricardoV94 I'd like to try implementing this. I just need some guidance. I don't quite understand what a shallow copy of a PYMC Model would entail. From what I understand, you can't mutate nested structures in a PYMC Model after you define the model. I think what clone_model() does is a deepcopy where making changes to nested structures like random variables that get turned into fixed values by using pm.do() won't be propagated to the clone.

@ricardoV94
Copy link
Member Author

We don't need a shallow copy, both methods can do a deep copy

pm.do itself also does a deepcopy before doing its business

@Dekermanjian
Copy link
Contributor

Oh okay, I see what you are saying. So both __copy__ and __deepcopy__ should return the same result that is equivalent to the clone_model() method. Did I understand that correctly?

@ricardoV94
Copy link
Member Author

Yup

@Dekermanjian
Copy link
Contributor

Okay, cool. Thanks!

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

Successfully merging a pull request may close this issue.

2 participants