-
Notifications
You must be signed in to change notification settings - Fork 510
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
Respect overwrite param in _merge_lists #3741
base: develop
Are you sure you want to change the base?
Conversation
if src is None: | ||
return | ||
|
||
if overwrite: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what led you to make this change in the first place; what were you merging? I can't think of any way to get data with a custom __eq__
method that ignores attribute values into a FiftyOne dataset
# equal, we want to make sure we persist src properties over dst | ||
dst, src = src, dst | ||
if not dst: | ||
dst = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't create a new array here because this function processes dst
in place. It doesn't return a new reference
# in cases where __eq__ has been defined such that two | ||
# instances without the exact same property values returns as | ||
# equal, we want to make sure we persist src properties over dst | ||
dst, src = src, dst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is subtle, but for core methods subtlety is important:
This implementation can cause the order of the elements in the destination list to change when overwrite=True
, as the source list is used as the starting point.
Conversely, in the implementation of _merge_labels()
below, I was careful to do it in such a way that any overwrites are done in-place, and any new elements are strictly appended to the end. So that the order of elements in the destination list will not change.
For example in Keypoint
labels there is a convention that the confidence
field contains a list of values that correspond 1-1 with the shapes defined in the points
field.
currently, passing overwrite in
_merge_lists
does nothing