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

248 cannot deepcopy model with nested list of constraints #250

Merged

Conversation

IgnaceBleukx
Copy link
Collaborator

Changed deepcopy functionality to work with copy.deepcopy()

@IgnaceBleukx IgnaceBleukx linked an issue Mar 8, 2023 that may be closed by this pull request
@tias
Copy link
Collaborator

tias commented Mar 8, 2023

Do we even need to overwrite the builtin deepcopy?

import copy
class cls:
    def __init__(self,a,b):
        self.a = a
        self.b = b
    
    def __repr__(self):
        return str((self.a, self.b))
x = cls("a", [1,2])
print(x)
print(copy.deepcopy(x))

this works... so, do we really need our own?

@IgnaceBleukx
Copy link
Collaborator Author

IgnaceBleukx commented Mar 9, 2023

Hmm, removing our custom __deepcopy__ indeed does not break any tests.
I looked around a little bit and it seems there are two main reasons why you would want to override __deepcopy__:

  1. To copy some attributes shallow and other deep (not applicable for us I think)
  2. If all objects should be made by a factory. I.e., a custom function that creates the objects, potentially with some bookkeeping. This is much like our intvar or boolvar function which ups our magic counters every time it is called. The __deepcopy__ functions should then call this constructor.
    However, when copying variables, we do not want the counter to be upped as the names should stay the same. So this case is also not applicable to our application.

So in summary, we can probably rely on the default implementation of copy.deepcopy and leave out our custom implementations...

@tias
Copy link
Collaborator

tias commented Mar 9, 2023

Fantastic code reduction : ) Do push and get back to/close Helge's

@tias tias merged commit e136483 into master Mar 10, 2023
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.

Cannot deepcopy model with nested list of constraints
2 participants