[FIX] Fix AbstractSortTableModel.mapFromSourceRows for empty list or array

[janezd]

Issue

Fixes #2728.

Description of changes

Test whether a list is empty before using it for indexing an array.

Includes

• Code changes
• Tests

[janezd]

@kernc, what do you prefer:

• (isinstance(rows, (int, type(Ellipsis))) or len(rows)),
• (not isinstance(rows, (list, np.ndarray)) or len(rows)), or
• (not isinstance(rows, collections.Sized) or len(rows))?

If the method is called with the argument types specified in the docstring, all are equivalent. Also, in cases where they are not equivalent (e.g. if rows is something stupid), the next line will fail anyway.

[kernc]

I already see:

>>> arr = np.ones(5)
>>> empty_list = []
>>> arr[empty_list]
array([], dtype=float64)

I.e. empty input, empty output.

You don't?

[kernc]

I would prefer:

if self.__sortInd is not None:
    if rows is not Ellipsis:
        rows = np.asarray(rows, dtype=int)  # dangerous casting

    new_rows = self.__sortInd[rows]
    ...

But this also relies on indexing (at least) with an empty array working.

I am not aware of this ever not being the case.

[kernc]

I would prefer your first option. Note to also fix .mapFromSourceRows() below.

[janezd]

dtype must not be float; the caller must ensure that. This is similar to __getitem__ which expects ints, and doesn't cast float to int. I made this clear in the docstring.

I fixed mapFromSourceRows().

[codecov-io]

Codecov Report

Merging #2730 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2730      +/-   ##
==========================================
+ Coverage   76.04%   76.05%   +<.01%     
==========================================
  Files         338      338              
  Lines       59643    59654      +11     
==========================================
+ Hits        45358    45370      +12     
+ Misses      14285    14284       -1

[kernc]

I think #2733 is a more correct fix.

[janezd]

If I understood your comments (and their removals) correctly, this is complete now. I also improved the tests for the two mappings, so they now actually test something. :)

[kernc]

Yes. Even so, I dared push some improvements while the build remains untamed.

[janezd]

Feel free to dare. I force-pushed, hope it builds now. (Sorry for removing your name from the commit; it disappeared while resolving merge conflicts(?!).)

[janezd]

AppVeyor:

Ran 1895 tests in 146.438s
OK (skipped=122)
Command exited with code -1073741819

Anybody, help?

[astaric]

This looks like a "segfault" to me, no idea why it happens though. It usually happens when an error occurs when destroying objects upon closing of the interpreter.

[kernc]

That's what one gets for squandering over sensible modifications.