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&test push/pull recarrays #2034

Merged
merged 4 commits into from Jun 25, 2012
Merged

fix&test push/pull recarrays #2034

merged 4 commits into from Jun 25, 2012

Conversation

minrk
Copy link
Member

@minrk minrk commented Jun 25, 2012

We were only pushing the dtype str for some reason,
which is totally wrong for recarrays.

closes #2033

@wkerzendorf
Copy link

man you're fast!! This only seems to happen on the pull. I can access things on the engines once they're pushed.

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

This is less efficient that what we used to do for simple arrays, as sending the array is ~200B instead of ~3, but that's not very much for sending a real array, and it should be much more general.

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

Test results for commit d8f2b33 merged into master
Platform: darwin

  • python2.6: Failed, log at https://gist.github.com/2986419 (libraries not available: cython matplotlib oct2py pygments pymongo qt rpy2 tornado wx wx.aui)
  • python2.7: OK (libraries not available: oct2py wx wx.aui)
  • python3.2: OK (libraries not available: cython matplotlib oct2py pymongo qt rpy2 wx wx.aui)

Not available for testing: python3.1

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

Hm, unpickling dtypes doesn't seem to work in numpy 1.5. Need to work on it for a moment more.

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

Okay, I think I got it for real this time. testing again...

@fperez
Copy link
Member

fperez commented Jun 25, 2012

Code looks clean, running test_pr right now. Thanks for catching this one, @wkerzendorf, and as usual record-setting response time, @minrk :)

@wkerzendorf
Copy link

No need to thank me, @minrk did all the work (and really fast), well if now I only have to get my own code working ;-)

'str' would fail with zero-size in numpy < 1.6
@fperez
Copy link
Member

fperez commented Jun 25, 2012

Test results for commit fc2de03 merged into master
Platform: linux2

Not available for testing: python2.6, python3.1

@fperez
Copy link
Member

fperez commented Jun 25, 2012

rerunning test_pr in case b54e738 helps with the failures reported above...

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

slow down - there's one more commit after b54.

@fperez
Copy link
Member

fperez commented Jun 25, 2012

ok, restarted run with bdf7984.

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

Hm, while I'm pretty sure bdf should work, I might take one more stab at sending not the dtype object itself.

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

Nevermind - I'll stick with the full dtype object since I'm not super confident in my understanding of dtype objects. I'll take a stab at cleaning it up when I do my general serialization improvement pass for 0.14.

@fperez
Copy link
Member

fperez commented Jun 25, 2012

Sounds good. Let's be conservative here, and we can ping Travis O. or Mark Wiebe in Austin for a bit of wisdom on possible improvements we could make here while remaining 100% correct.

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

Test results for commit bdf7984 merged into master
Platform: darwin

  • python2.6: OK (libraries not available: cython matplotlib oct2py pygments pymongo qt rpy2 tornado wx wx.aui)
  • python2.7: OK (libraries not available: oct2py wx wx.aui)
  • python3.2: OK (libraries not available: cython matplotlib oct2py pymongo qt rpy2 wx wx.aui)

Not available for testing: python3.1

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

It's relatively minor - with this change, we are sending a few hundred bytes of metadata when ~99% of cases are covered with less than five.

I'm pretty sure the right answer is:

dtype = array.dtype.descr if array.dtype.shape else array.dtype.str

But I don't want to make a change like that immediately before release.

@fperez
Copy link
Member

fperez commented Jun 25, 2012

Test results for commit bdf7984 merged into master
Platform: linux2

  • python2.7: OK
  • python3.2: OK (libraries not available: cython matplotlib oct2py pygments pymongo rpy2 wx wx.aui)

Not available for testing: python2.6, python3.1

@fperez
Copy link
Member

fperez commented Jun 25, 2012

Conservative but safe, and code looks clean; thanks for the added tests! Merging now, glad we caught this before 0.13!

fperez added a commit that referenced this pull request Jun 25, 2012
fix&test push/pull recarrays: We were only pushing the dtype str for some reason, which is totally wrong for recarrays.

closes #2033
@fperez fperez merged commit feac402 into ipython:master Jun 25, 2012
@fperez
Copy link
Member

fperez commented Jun 25, 2012

Perhaps we should leave an issue open on this, or is it 'in cache' enough that you want to just open the 0.14 PR with that change?

@minrk
Copy link
Member Author

minrk commented Jun 25, 2012

I'll add a note to #1504, which is for the more general serialization adjustments (and for which I already have a months-old branch in-progress)

@fperez
Copy link
Member

fperez commented Jun 25, 2012

Perfect, thanks.

@minrk minrk deleted the recarray branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
fix&test push/pull recarrays: We were only pushing the dtype str for some reason, which is totally wrong for recarrays.

closes ipython#2033
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.

ipython parallel pushing and pulling recarrays
3 participants