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

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

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Nov 2, 2017

Issue

Fixes #2728.

Description of changes

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

Includes
  • Code changes
  • Tests

@janezd janezd force-pushed the fix-mapFromSourceRows branch from 1cbeae3 to 4ccaaec Compare November 2, 2017 10:22
# self.__sortInd[rows] fails if `rows` is an empty list or array
if self.__sortInd is not None \
and (isinstance(rows, (int, type(Ellipsis)))
or len(rows)):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@kernc kernc Nov 2, 2017

Choose a reason for hiding this comment

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

I already see:

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

I.e. empty input, empty output.

You don't?

Copy link
Contributor

@kernc kernc Nov 2, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link

codecov-io commented Nov 2, 2017

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
Copy link
Contributor

kernc commented Nov 2, 2017

I think #2733 is a more correct fix.

@janezd janezd force-pushed the fix-mapFromSourceRows branch 4 times, most recently from 911f556 to 0f53884 Compare November 2, 2017 20:54
@janezd
Copy link
Contributor Author

janezd commented Nov 2, 2017

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 kernc force-pushed the fix-mapFromSourceRows branch from 0f53884 to 54c5e9f Compare November 3, 2017 01:20
@kernc
Copy link
Contributor

kernc commented Nov 3, 2017

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

@janezd janezd force-pushed the fix-mapFromSourceRows branch from 54c5e9f to d83130c Compare November 3, 2017 07:35
@janezd
Copy link
Contributor Author

janezd commented Nov 3, 2017

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 janezd force-pushed the fix-mapFromSourceRows branch 2 times, most recently from 6a25001 to 6d6ff33 Compare November 3, 2017 08:12
@janezd
Copy link
Contributor Author

janezd commented Nov 3, 2017

AppVeyor:

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

Anybody, help?

@kernc kernc force-pushed the fix-mapFromSourceRows branch from 6d6ff33 to fcb2da3 Compare November 3, 2017 11:38
@astaric
Copy link
Member

astaric commented Nov 3, 2017

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 kernc force-pushed the fix-mapFromSourceRows branch from fcb2da3 to 5854208 Compare November 3, 2017 12:04
@kernc
Copy link
Contributor

kernc commented Nov 3, 2017

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

@kernc kernc merged commit 727430c into biolab:master Nov 3, 2017
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