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

[BUG] Fix bug in trimr when inclusive=False #10549

Merged
merged 3 commits into from
Aug 8, 2019
Merged

Conversation

makbigc
Copy link
Contributor

@makbigc makbigc commented Jul 29, 2019

when inclusive=False, mstats.trimr runs into TypeError i.e., TypeError: slice indices must be integers or None or have an __index__ method.

import numpy.ma as ma
import scipy.stats.mstats as mstats

a = ma.arange(10)
mstats.trimr(a, limits=(0.15, 0.14), inclusive=(False, False)

@chrisb83 chrisb83 added maintenance Items related to regular maintenance tasks scipy.stats labels Jul 31, 2019
x = ma.arange(10)
result = mstats.trimr(x, limits=(0.15, 0.14), inclusive=(False, False))
expected = [None, None, 2, 3, 4, 5, 6, 7, 8, None]
assert_equal(result, expected)
Copy link
Member

Choose a reason for hiding this comment

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

result is a masked array, apparently the comparison in assert_equal with the list works. I did not check in the other Tests: is that the standard way to write the test cases for masked arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I follow the format in test_trim to write the present test.
assert_equal is stricter than other test utilities such as assert_almost_equal.

Copy link
Member

@pv pv Aug 4, 2019

Choose a reason for hiding this comment

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

iirc numpy test functions had stuff in them for masked array handling, and will cast inputs to ndarrays if either one is

Copy link
Member

@pv pv Aug 4, 2019

Choose a reason for hiding this comment

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

Actually, not exactly:

>>> x = np.ma.array([1,2,3,4,5], dtype=int, mask=[0,0,0,1,0])
>>> numpy.testing.assert_equal(x, [1,2,3,None,5])
>>> numpy.testing.assert_equal(x, [1,2,3,4,5])
>>> numpy.testing.assert_equal(x, [1,2,3,999,5])

It won't check that the mask is correct, so this test should check the mask separately (which some of the tests above do).

@pv pv added the needs-work Items that are pending response from the author label Aug 4, 2019
result = mstats.trimr(x, limits=(0.15, 0.14), inclusive=(False, False))
expected = ma.array([0, 1, 2, 3, 4, 5, 6, 7, 8, 9],
mask=[1, 1, 0, 0, 0, 0, 0, 0, 0, 1])
pytest.set_trace()
Copy link
Member

Choose a reason for hiding this comment

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

Forgot set_trace here

@pv pv added needs-work Items that are pending response from the author and removed needs-work Items that are pending response from the author labels Aug 5, 2019
Copy link
Member

@chrisb83 chrisb83 left a comment

Choose a reason for hiding this comment

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

Test looks good now, waiting for @pv to confirm

@pv pv merged commit 284b020 into scipy:master Aug 8, 2019
@pv pv added this to the 1.4.0 milestone Aug 8, 2019
@tylerjereddy tylerjereddy removed the needs-work Items that are pending response from the author label Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.stats
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants