Skip to content

Commit

Permalink
Fix slices handling in LazyList (#8299)
Browse files Browse the repository at this point in the history
<!-- Raise an issue to propose your change
(https://github.com/cvat-ai/cvat/issues).
It helps to avoid duplication of efforts from multiple independent
contributors.
Discuss your ideas with maintainers to be sure that changes will be
approved and merged.
Read the [Contribution guide](https://docs.cvat.ai/docs/contributing/).
-->

<!-- Provide a general summary of your changes in the Title above -->

### Motivation and context
Following
[v2.16.2](https://github.com/cvat-ai/cvat/releases/tag/v2.16.2) release,
we noticed an issue with annotations export with CVAT format and masks.

Upon investigation, we found that the problem stemmed from changes
introduced in this PR: #8229.

The issue was caused by the improper handling of slices with negative
positions in LazyList, particularly affecting the computation of
resulting data. This behaviour was triggered by the following line:

https://github.com/cvat-ai/cvat/blob/cfce7be5a9422da2c367296f56d70181185f503e/cvat/apps/dataset_manager/formats/cvat.py#L797


<!-- Why is this change required? What problem does it solve? If it
fixes an open
issue, please link to the issue here. Describe your changes in detail,
add
screenshots. -->

### How has this been tested?
<!-- Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc. -->

### Checklist
<!-- Go over all the following points, and put an `x` in all the boxes
that apply.
If an item isn't applicable for some reason, then ~~explicitly
strikethrough~~ the whole
line. If you don't do that, GitHub will show incorrect progress for the
pull request.
If you're unsure about any of these, don't hesitate to ask. We're here
to help! -->
- [x] I submit my changes into the `develop` branch
- [x] I have created a changelog fragment <!-- see top comment in
CHANGELOG.md -->
- [ ] I have updated the documentation accordingly
- [x] I have added tests to cover my changes
- [ ] I have linked related issues (see [GitHub docs](

https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword))
- [ ] I have increased versions of npm packages if it is necessary

([cvat-canvas](https://github.com/cvat-ai/cvat/tree/develop/cvat-canvas#versioning),

[cvat-core](https://github.com/cvat-ai/cvat/tree/develop/cvat-core#versioning),

[cvat-data](https://github.com/cvat-ai/cvat/tree/develop/cvat-data#versioning)
and

[cvat-ui](https://github.com/cvat-ai/cvat/tree/develop/cvat-ui#versioning))

### License

- [x] I submit _my code changes_ under the same [MIT License](
https://github.com/cvat-ai/cvat/blob/develop/LICENSE) that covers the
project.
  Feel free to contact the maintainers if that's a concern.


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Enhanced handling of negative indices in list slicing, improving
robustness and performance.
- Added new test methods to validate slicing operations, ensuring
correct behavior for various scenarios.

- **Bug Fixes**
- Adjusted existing tests to account for empty values in the list,
improving accuracy of expected outputs.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
Bobronium authored Aug 13, 2024
1 parent 3b568b5 commit 5314bda
Show file tree
Hide file tree
Showing 4 changed files with 262 additions and 218 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### Fixed

- Fixed issue with slices handling in `LazyList` which caused problems with exporting masks
in `CVAT for images 1.1` format.
(<https://github.com/cvat-ai/cvat/pull/8299>)
21 changes: 18 additions & 3 deletions cvat/apps/engine/lazy_list.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,21 @@ def __getitem__(self, index: int | slice) -> T | list[T]:
return list.__getitem__(self, index)

if isinstance(index, slice):
self._parse_up_to(index.indices(self._compute_max_length(self._string))[1] - 1)
if (
index.start is not None
and index.start < 0
or index.stop is not None
and index.stop < 0
or index.step is not None
and index.step < 0
):
# to compute negative indices we must know the exact length in advance
# which is impossible if we take into account missing elements,
# so we have to parse the full list
self._parse_up_to(-1)
else:
self._parse_up_to(index.indices(self._compute_max_length(self._string))[1] - 1)

return list.__getitem__(self, index)

self._parse_up_to(index)
Expand Down Expand Up @@ -194,7 +208,8 @@ def _iter_unparsed(self):
for _ in range(current_index):
current_position = string.find(self._separator, current_position) + separator_offset

while current_index < self._compute_max_length(string):
probable_length = self._compute_max_length(string)
while current_index < probable_length:
end = string.find(self._separator, current_position, string_length)
if end == -1:
end = string_length
Expand All @@ -203,7 +218,7 @@ def _iter_unparsed(self):
element_str = string[current_position:end]
current_position = end + separator_offset
if not element_str:
self._probable_length -= 1
probable_length -= 1
continue
element = self._converter(element_str)
if list.__len__(self) <= current_index:
Expand Down
134 changes: 79 additions & 55 deletions cvat/apps/engine/tests/test_lazy_list.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,70 +8,73 @@
T = TypeVar('T')


class SliceGetter:
def __class_getitem__(cls, slice_: slice) -> slice:
if not isinstance(slice_, slice):
raise TypeError("Use slice_getter only for slices")
return slice_


class TestLazyList(unittest.TestCase):

def setUp(self):
self.lazy_list = LazyList(string="1,2,3", converter=int)

def test_skipped_values(self):
ll = LazyList("1,2,,4", converter=int)
self.assertEqual(len(ll), 3)
self.assertEqual(ll, [1, 2, 4])
self.lazy_list = LazyList(string="1,2,,3,4,5,6,7,8,9,10,11,12,,13,14,15", converter=int)
self.empty_lazy_list = LazyList(string="")

def test_len(self):
self.assertEqual(len(self.lazy_list), 3)
self.assertEqual(len(self.lazy_list), 15)
list(self.lazy_list)
self.assertEqual(len(self.lazy_list), 3)
self.assertEqual(len(self.lazy_list), 15)

def test_repr(self):
self.assertEqual(repr(self.lazy_list), "LazyList([... + 1,2,3', (0.00% parsed))")
self.assertEqual(repr(self.lazy_list), "LazyList([... + 1,2,,3,4,5,6,7,8,9,10,11,12,,13,14,15', (0.00% parsed))")
next(iter(self.lazy_list)) # Trigger parsing of the first element
self.assertIn("1... + 2,3", repr(self.lazy_list))
self.assertIn("1... + 2,,3,4", repr(self.lazy_list))
list(self.lazy_list)
self.assertEqual(repr(self.lazy_list), "LazyList([1, 2, 3])")
self.assertEqual(repr(self.lazy_list), "LazyList([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])")

def test_deepcopy(self):
copied_list = copy.deepcopy(self.lazy_list)
self.assertEqual(copied_list, [1, 2, 3])
self.assertEqual(copied_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])
self.assertNotEqual(id(copied_list), id(self.lazy_list))
self.assertEqual(len(copied_list), 3)
self.assertEqual(len(copied_list), 15)

def test_getitem(self):
self.assertEqual(self.lazy_list[1], 2)
self.assertEqual(self.lazy_list[:2], [1, 2])
self.assertEqual(self.lazy_list[:3], [1, 2, 3])

def test_iter(self):
iterator = iter(self.lazy_list)
self.assertEqual(next(iterator), 1)
self.assertEqual(next(iterator), 2)
self.assertEqual(list(self.lazy_list), [1, 2, 3])
self.assertEqual(list(self.lazy_list), [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])
self.assertEqual(next(iterator), 3)
self.assertEqual(list(self.lazy_list), [1, 2, 3])
self.assertEqual(list(self.lazy_list), [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_append(self):
self.lazy_list.append(4)
self.assertEqual(self.lazy_list, [1, 2, 3, 4])
self.lazy_list.append(16)
self.assertEqual(self.lazy_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16])

def test_copy(self):
copied_list = self.lazy_list.copy()
self.assertEqual(copied_list, [1, 2, 3])
self.assertEqual(copied_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_insert(self):
self.lazy_list.insert(0, 0)
self.assertEqual(self.lazy_list, [0, 1, 2, 3])
self.assertEqual(self.lazy_list, [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_pop(self):
value = self.lazy_list.pop()
self.assertEqual(value, 3)
self.assertEqual(self.lazy_list, [1, 2])
self.assertEqual(value, 15)
self.assertEqual(self.lazy_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14])

def test_remove(self):
self.lazy_list.remove(2)
self.assertEqual(self.lazy_list, [1, 3])
self.assertEqual(self.lazy_list, [1, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_reverse(self):
self.lazy_list.reverse()
self.assertEqual(self.lazy_list, [3, 2, 1])
self.assertEqual(self.lazy_list, [15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1])

def test_sort(self):
unsorted_list = LazyList(string="3,1,2", converter=int)
Expand All @@ -83,101 +86,122 @@ def test_clear(self):
self.assertEqual(len(self.lazy_list), 0)

def test_index(self):
self.assertEqual(self.lazy_list.index(2), 1)
self.assertEqual(self.lazy_list.index(3), 2)

def test_count(self):
self.assertEqual(self.lazy_list.count(2), 1)

def test_setitem(self):
self.lazy_list[0] = 4
self.assertEqual(self.lazy_list[0], 4)
self.lazy_list[0] = 99
self.assertEqual(self.lazy_list[0], 99)

def test_delitem(self):
del self.lazy_list[0]
self.assertEqual(self.lazy_list, [2, 3])
self.assertEqual(self.lazy_list, [2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_contains(self):
self.assertIn(2, self.lazy_list)

def test_reversed(self):
self.assertEqual(list(reversed(self.lazy_list)), [3, 2, 1])
self.assertEqual(list(reversed(self.lazy_list)), [15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1])

def test_mul(self):
self.assertEqual(self.lazy_list * 2, [1, 2, 3, 1, 2, 3])
self.assertEqual(self.lazy_list * 2, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_rmul(self):
self.assertEqual(2 * self.lazy_list, [1, 2, 3, 1, 2, 3])
self.assertEqual(2 * self.lazy_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_imul(self):
self.lazy_list *= 2
self.assertEqual(self.lazy_list, [1, 2, 3, 1, 2, 3])
self.assertEqual(self.lazy_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_extend(self):
self.lazy_list.extend([4, 5])
self.assertEqual(self.lazy_list, [1, 2, 3, 4, 5])
self.lazy_list.extend([16, 17])
self.assertEqual(self.lazy_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17])

def test_add(self):
new_list = self.lazy_list + [4, 5]
self.assertEqual(new_list, [1, 2, 3, 4, 5])
new_list = self.lazy_list + [16, 17]
self.assertEqual(new_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17])

def test_eq(self):
self.assertTrue(self.lazy_list == [1, 2, 3])
self.assertTrue(self.lazy_list == [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_iadd(self):
self.lazy_list += [4, 5]
self.assertEqual(self.lazy_list, [1, 2, 3, 4, 5])
self.lazy_list += [16, 17]
self.assertEqual(self.lazy_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17])

def test_gt(self):
self.assertTrue(self.lazy_list > [1, 2])
self.assertTrue(self.lazy_list > [1, 2, 3])

def test_ge(self):
self.assertTrue(self.lazy_list >= [1, 2, 3])
self.assertTrue(self.lazy_list >= [1, 2, 3, 4, 5])

def test_lt(self):
self.assertTrue(self.lazy_list < [1, 2, 3, 4])
self.assertTrue(self.lazy_list < [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16])

def test_le(self):
self.assertTrue(self.lazy_list <= [1, 2, 3])
self.assertTrue(self.lazy_list <= [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_lazy_list_with_lazy_list(self):
other_lazy_list = LazyList(string="4,5,6", converter=int)
other_lazy_list = LazyList(string="16,17", converter=int)
combined_list = self.lazy_list + other_lazy_list
self.assertEqual(combined_list, [1, 2, 3, 4, 5, 6])
self.assertEqual(combined_list, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17])

def test_pickle_support(self):
pickled = pickle.dumps(self.lazy_list)
unpickled = pickle.loads(pickled)
self.assertEqual(unpickled, [1, 2, 3])
self.assertEqual(unpickled, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])
self.assertEqual(unpickled._string, "")
self.assertTrue(unpickled._parsed)

def test_parse_before_accessing_decorator(self):
lazy_list_copy = LazyList(string="1,2,3", converter=int)
lazy_list_copy.append(4)
self.assertEqual(lazy_list_copy, [1, 2, 3, 4])
lazy_list_copy = LazyList(string="1,2,,3,4,5,6,7,8,9,10,11,12,,13,14,15", converter=int)
lazy_list_copy.append(16)
self.assertEqual(lazy_list_copy, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16])

def test_parse_both_before_accessing_decorator(self):
other_list = LazyList(string="4,5", converter=int)
other_list = LazyList(string="16,17", converter=int)
result = self.lazy_list + other_list
self.assertEqual(result, [1, 2, 3, 4, 5])
self.assertEqual(result, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17])

def test_length_on_iteration(self):
elements = []
for element in self.lazy_list:
self.assertEqual(len(self.lazy_list), 3)
self.assertEqual(len(self.lazy_list), 15)
elements.append(element)

self.assertEqual(elements, [1, 2, 3])
self.assertEqual(elements, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15])

def test_str(self):
self.assertEqual(str(self.lazy_list), "1,2,3")
self.assertEqual(str(self.lazy_list), "1,2,,3,4,5,6,7,8,9,10,11,12,,13,14,15")
self.assertEqual(self.lazy_list, LazyList(str(self.lazy_list), converter=int))

def test_str_parsed(self):
list(self.lazy_list)
self.assertEqual(str(self.lazy_list), "1,2,3")
self.assertEqual(str(self.lazy_list), "1,2,3,4,5,6,7,8,9,10,11,12,13,14,15")
self.assertEqual(self.lazy_list, LazyList(str(self.lazy_list), converter=int))

def test_slice(self):
for slice_, expected in (
(SliceGetter[1:], [2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]),
(SliceGetter[:3], [1, 2, 3]),
(SliceGetter[::2], [1, 3, 5, 7, 9, 11, 13, 15]),
(SliceGetter[1::2], [2, 4, 6, 8, 10, 12, 14]),
(SliceGetter[::-1], [15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1]),
(SliceGetter[-2:], [14, 15]),
(SliceGetter[:-2], [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]),
(SliceGetter[:-1], [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14]),
(SliceGetter[-2::-1], [14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1]),
(SliceGetter[::2], [1, 3, 5, 7, 9, 11, 13, 15]),
(SliceGetter[::-2], [15, 13, 11, 9, 7, 5, 3, 1]),
(SliceGetter[1::-1], [2, 1]),
(SliceGetter[1:3:2], [2]),
):
with self.subTest(f"self.lazy_list[{slice_}]. Expected: {expected}"):
self.assertEqual(self.lazy_list[slice_], expected)
with self.subTest(f"self.empty_lazy_list[{slice_}]. Expected: []"):
self.assertEqual(self.empty_lazy_list[slice_], [])


if __name__ == "__main__":
unittest.main()
Loading

0 comments on commit 5314bda

Please sign in to comment.