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

Python copy support #1575

Merged
merged 7 commits into from
Jan 13, 2022
Merged

Conversation

remia
Copy link
Collaborator

@remia remia commented Dec 22, 2021

This PR addresses #1434 adding copy / deepcopy (same implementation) for OCIO objects having C++ createEditableCopy() methods. It also fix the typo reported in #1540.

Copy link
Collaborator

@zachlewis zachlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Remi, this is super useful. Seems to work as expected.

@hodoulp
Copy link
Member

hodoulp commented Jan 7, 2022

[Question] Only the deep copy exists in the public API of OpenColorIO i.e. createEditableCopy(), why also adding the Python copy()?

@remia
Copy link
Collaborator Author

remia commented Jan 7, 2022

Good question.

It seems that in our context, the difference between Python copy() and deepcopy() is not relevant and both may be accurately implemented with createEditableCopy(). At least that's how I understand it because I don't have a clear view on the object memory layout as generated by pybind11 and it may be unnecessary complexity to strictly follow Python definition of shallow copy.

Here is the pybind11 doc and a quote from the copy module doc:

The difference between shallow and deep copying is only relevant for compound objects (objects that contain other objects, like lists or class instances):

  • A shallow copy constructs a new compound object and then (to the extent possible) inserts references into it to the objects found in the original.
  • A deep copy constructs a new compound object and then, recursively, inserts copies into it of the objects found in the original.

@hodoulp
Copy link
Member

hodoulp commented Jan 7, 2022

There is a major semantic difference between shallow and deep copies and, adding the two methods in Python introduces a discrepancy between the Python and C++ APIs (because the shallow copy is not supported in the C++ API). For example, a deep copy of a config instance means that the file rules are not shared between the two config instances (where a shallow copy would have share the file rule instance).

I think that adding the Python copy() (i.e. a shallow copy) introduces some confusion because it's in fact implemented by a deep copy.

@remia
Copy link
Collaborator Author

remia commented Jan 7, 2022

I can remove it, it will go back to produce a segfault as before if we are fine with it.

Edit: I removed the __copy__ method to avoid any confusion as suggested by @hodoulp.

Copy link
Member

@hodoulp hodoulp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only made some comments for code improvements.

tests/python/ConfigTest.py Outdated Show resolved Hide resolved
tests/python/ConfigTest.py Show resolved Hide resolved
tests/python/ConfigTest.py Outdated Show resolved Hide resolved
tests/python/ConfigTest.py Outdated Show resolved Hide resolved
tests/python/ContextTest.py Outdated Show resolved Hide resolved
tests/python/ConfigTest.py Show resolved Hide resolved
Signed-off-by: Rémi Achard <[email protected]>
@hodoulp hodoulp merged commit d5bf1e1 into AcademySoftwareFoundation:main Jan 13, 2022
hodoulp added a commit to hodoulp/OpenColorIO that referenced this pull request Jan 13, 2022
* Add deepcopy support for object with createEditableCopy method

Signed-off-by: Rémi Achard <[email protected]>

* Minor fixes

Signed-off-by: Rémi Achard <[email protected]>

* Removing python shallow copy method

Signed-off-by: Rémi Achard <[email protected]>

* Update comment and formating

Signed-off-by: Rémi Achard <[email protected]>

* Testing COnfig FileRules deepCopy

Signed-off-by: Rémi Achard <[email protected]>

* Add comment for ConfigTest

Signed-off-by: Rémi Achard <[email protected]>

Co-authored-by: Patrick Hodoul <[email protected]>
hodoulp added a commit that referenced this pull request Jan 24, 2022
* Add deepcopy support for object with createEditableCopy method

Signed-off-by: Rémi Achard <[email protected]>

* Minor fixes

Signed-off-by: Rémi Achard <[email protected]>

* Removing python shallow copy method

Signed-off-by: Rémi Achard <[email protected]>

* Update comment and formating

Signed-off-by: Rémi Achard <[email protected]>

* Testing COnfig FileRules deepCopy

Signed-off-by: Rémi Achard <[email protected]>

* Add comment for ConfigTest

Signed-off-by: Rémi Achard <[email protected]>

Co-authored-by: Patrick Hodoul <[email protected]>

Co-authored-by: Rémi Achard <[email protected]>
@remia remia deleted the python-copy-support branch January 25, 2022 19:26
hodoulp added a commit to hodoulp/OpenColorIO that referenced this pull request Mar 8, 2022
* Add deepcopy support for object with createEditableCopy method

Signed-off-by: Rémi Achard <[email protected]>

* Minor fixes

Signed-off-by: Rémi Achard <[email protected]>

* Removing python shallow copy method

Signed-off-by: Rémi Achard <[email protected]>

* Update comment and formating

Signed-off-by: Rémi Achard <[email protected]>

* Testing COnfig FileRules deepCopy

Signed-off-by: Rémi Achard <[email protected]>

* Add comment for ConfigTest

Signed-off-by: Rémi Achard <[email protected]>

Co-authored-by: Patrick Hodoul <[email protected]>
hodoulp added a commit that referenced this pull request Mar 11, 2022
* Python copy support (#1575)

* Add deepcopy support for object with createEditableCopy method

Signed-off-by: Rémi Achard <[email protected]>

* Minor fixes

Signed-off-by: Rémi Achard <[email protected]>

* Removing python shallow copy method

Signed-off-by: Rémi Achard <[email protected]>

* Update comment and formating

Signed-off-by: Rémi Achard <[email protected]>

* Testing COnfig FileRules deepCopy

Signed-off-by: Rémi Achard <[email protected]>

* Add comment for ConfigTest

Signed-off-by: Rémi Achard <[email protected]>

Co-authored-by: Patrick Hodoul <[email protected]>

* Unit test fix

Signed-off-by: Patrick Hodoul <[email protected]>

Co-authored-by: Rémi Achard <[email protected]>
@doug-walker doug-walker added this to the OCIO 2.2.0 milestone Apr 6, 2022
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.

4 participants