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: series timedelta arithmetic is not being converted to ns with numpy 1.6 #4138

Closed
wants to merge 1 commit into from
Closed

BUG: series timedelta arithmetic is not being converted to ns with numpy 1.6 #4138

wants to merge 1 commit into from

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Jul 5, 2013

closes #4135.

@ghost ghost assigned cpcloud Jul 5, 2013
@michaelaye
Copy link
Contributor

Does the Travis fail mean that it will not be considered for merge?

@jreback
Copy link
Contributor

jreback commented Jul 8, 2013

no that's unrelated

just didn't get to it

@cpcloud
Copy link
Member Author

cpcloud commented Jul 8, 2013

this was failing for numpy running on python 2.6 having to do with timedelta64's arguments need to figure out what is up with that.

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

bug here is a bit more subtle because of numpy 1.6.x's different timedelta/datetime API.

@michaelaye
Copy link
Contributor

is Travis having numpy 1.6 with Python 2.6 only? Because I was using numpy 1.6 with Python 2.7 for, I believe, years! So that would be a much more realistic use case.

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

@michaelaye see #4153 (comment)

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

short is answer is no, it's not :)

@michaelaye
Copy link
Contributor

ok, so that means the fail with the 2.6 env (or better to say locale?) can not have anything to do with numpy 1.6.1, because it works in 2.7, right?

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

yes that's a different issue, as i pointed out above. before i was passing 2 args to timedelta64 which is invalid in numpy 1.6.x, now there's something else going on.

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

@michaelaye no, in fact, i'm almost 100% sure it has something to do with 1.6.x. look at how the tests are run....

  • 2.6 runs all tests that don't require full dependencies and uses 1.6.1: FAIL
  • 2.7 without LOCALE runs all tests with full deps and uses 1.7.1: PASS
  • 2.7 with locale runs slow and not network tests, and uses 1.6.1: PASS (because it doesn't run my new test)
  • 3.2 same as 2.6 but with full deps and numpy 1.6.2: FAIL
  • 3.3 numpy 1.7.1: PASS

@jreback
Copy link
Contributor

jreback commented Jul 9, 2013

1.6.2 IS different (buggy basically).....will have to look at this

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

@michaelaye
Copy link
Contributor

oh, but the 3.2 env did work before, no? I thought I saw before that 2.6 env was the only one failing, so that must be your new test, I guess. (I am a noob about Travis, need to learn this ...)

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

yep. before it was failing because you can't pass units to old timedelta (raising the standard TypeError when you call a function with more arguments than it has formal parameters), so i just manually did it by taking the dot product of the unit number of microseconds with the coefficients in my test (old timedelta doesn't support lower than microseconds), now pandas is raising

@jreback
Copy link
Contributor

jreback commented Jul 9, 2013

just to make it even harder
python 3 is completely different
treats timedelta more like ints

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

joy

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

i think the type testing function is being too strict...checking it out now

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

btw, we really should try and reduce the build time of the cython stuff by using fused types

@michaelaye
Copy link
Contributor

That's actually a question I had on my mind for months: I see that homebrew can build using all cores, but the pandas built ever only uses 1 core. How can one improve that?

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

i think i fixed the arith issue, i wasn't converting to timedelta in the test...we'll see what happens

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

@michaelaye not sure about that...i don't think you can parallelize that (easily) since setup.py is what is controlling the compilation, so the gcc process has python as it's parent process and thus i think you'd need some way to tell python that it's ok to start up multiple gcc processes on independent files

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

but really i was referring to algos and things made by generated.py because that's where most of the bottleneck is and most of the repetition across types. right now pandas is generating types manually where fused types are similar to templates they allow you to write the code once and let the compiler (Cython) take care of generating the code for the different types. what's kind of cool is that you can use the function like a dict where the keys are types and the values are the specific function for that type which might allow you to fine tune if you needed to, at a higher level

@cpcloud
Copy link
Member Author

cpcloud commented Jul 9, 2013

arg now it's failing because of something that looks minor. can't look at it right now ...

if value.dtype != 'timedelta64[ns]':
if np.__version__ < LooseVersion('1.7'):
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe put this at the top of the file?

@cpcloud
Copy link
Member Author

cpcloud commented Aug 3, 2013

update:

going to get to the bottom of why this works this weekend and if everything makes sense i'll write it up here and merge

@jreback
Copy link
Contributor

jreback commented Aug 12, 2013

closing in favor of #4534

@jreback jreback closed this Aug 12, 2013
@cpcloud
Copy link
Member Author

cpcloud commented Aug 12, 2013

thanks

@cpcloud cpcloud deleted the series-timedelta-arith-fix branch August 12, 2013 22:46
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.

BUG: np.timedelta64 arithmetic appears buggy
3 participants