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

Index out of bounds error raised for negative indices. #3168

Closed
wants to merge 1 commit into from

Conversation

da415
Copy link

@da415 da415 commented Mar 25, 2013

Accessing out of bounds negative timeseries indices silently return nonsense values.

Accessing out of bounds negative timeseries indices silently return nonsense values.
@jreback
Copy link
Contributor

jreback commented Mar 25, 2013

@da415 can you put an example of this change?

also just check that performance is unchanged
e.g. run test_perf.sh

@ghost
Copy link

ghost commented Mar 26, 2013

Looks like the cython (no) bounds-checking monster has roared again.
#3028 , #3029

I get a segfault rather then junk data.

In [6]: s=mkdf(10,5,r_idx_type='dt').C_l0_g0
   ...: s
Out[6]: 
R0
2000-01-03    R0C0
2000-01-04    R1C0
2000-01-05    R2C0
2000-01-06    R3C0
2000-01-07    R4C0
2000-01-10    R5C0
2000-01-11    R6C0
2000-01-12    R7C0
2000-01-13    R8C0
2000-01-14    R9C0
Freq: B, Name: C_l0_g0, dtype: object

In [7]: s[0]
Out[7]: 'R0C0'

In [8]: s[-1]
Out[8]: 'R9C0'

In [9]: s[-11]
** segfault **

#3029 doesn't seem to catch this either, @stephenwlin?

@jreback
Copy link
Contributor

jreback commented Mar 26, 2013

I think this is very easy to fix by putting some minimal error checking in Series.getitem , and using indexing._maybe_convert_indicies (like how we are handling take which is user facing), I think the perf impact will be quite minimal then, rather than trying to catch this in cython (which is very general)

@jreback
Copy link
Contributor

jreback commented Mar 26, 2013

In fact, this is maybe an excuse to do some more unification on series/frame getitem/setitem, @stephenwlin made them nice and user friendly......prob not too hard to 'fix' series

@ghost
Copy link

ghost commented Mar 26, 2013

sounds good.

@jreback
Copy link
Contributor

jreback commented Mar 26, 2013

not sure will have time to get out before 0.11...shall we push to 0.12? @wesm

@jreback
Copy link
Contributor

jreback commented Mar 27, 2013

@da415 I believe that your solution fails the tseries/test_resample test..can you confirm?

@da415
Copy link
Author

da415 commented Mar 28, 2013

@jreback Apologies, the earliest I can confirm this failure is Tuesday 02/04.

@jreback
Copy link
Contributor

jreback commented Mar 28, 2013

@da415 sorry...didn't mean that...I tried your solution and it fails test_resample (on your commit); this was for sure a problem all along! thanks for catching it. I was just trying to see if maybe I am seeing something different.

@wesm
Copy link
Member

wesm commented Mar 28, 2013

I fixed it directly in the segfaulting function and added test case. thanks

@wesm wesm closed this Mar 28, 2013
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.

3 participants