Respect overwrite param in _merge_lists #3741
Conversation
| if src is None: | ||
| return | ||
|
|
||
| if overwrite: |
There was a problem hiding this comment.
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.
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.
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_listsdoes nothing