Pickling support added. Various whitespace fixes as a result of reading *lots* of code. #1175

Merged
merged 3 commits into from Sep 1, 2012

Conversation

Projects
None yet
7 participants
@pelson
Member

pelson commented Aug 30, 2012

Rebasing #1020 was causing real problems, therefore I decided to cut a fresh branch and merge my changes on (with a commit squash to boot).

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Aug 30, 2012

Member

I see changes to colorbar and legend! Does this mean you've added support for them?

Member

dmcdougall commented Aug 30, 2012

I see changes to colorbar and legend! Does this mean you've added support for them?

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Aug 30, 2012

Contributor

This new new_figure_manager_given_figure method should be added to backend_pgf.py as well, right?

Contributor

pwuertz commented Aug 30, 2012

This new new_figure_manager_given_figure method should be added to backend_pgf.py as well, right?

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Aug 30, 2012

This pull request fails (merged 51f8ffc6 into cf7618c).

This pull request fails (merged 51f8ffc6 into cf7618c).

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 30, 2012

Member

@dmcdougall: Yes. ;-)
@pwuertz: Yes. ;-)

Will do the pgf backend now.

Member

pelson commented Aug 30, 2012

@dmcdougall: Yes. ;-)
@pwuertz: Yes. ;-)

Will do the pgf backend now.

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Aug 30, 2012

This pull request fails (merged 7885d94 into cf7618c).

This pull request fails (merged 7885d94 into cf7618c).

+ if getattr(self.canvas, 'manager', None) is not None:
+ manager = self.canvas.manager
+ import matplotlib._pylab_helpers
+ if manager in matplotlib._pylab_helpers.Gcf.figs.viewvalues():

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 30, 2012

Member

based on the travis results, I am assuming viewvalues is not python2.6 compatible?

@pelson

pelson Aug 30, 2012

Member

based on the travis results, I am assuming viewvalues is not python2.6 compatible?

This comment has been minimized.

Show comment Hide comment
@astrofrog

astrofrog Aug 30, 2012

Contributor

Yes, this was added as part of Python 2.7 (http://docs.python.org/dev/whatsnew/2.7.html#pep-3106-dictionary-views).

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 30, 2012

Member

I have been developing this with python2.7, but it appears that this is not compatible with python3.2 nor python2.6. I don't have local installations of either and would have to build both from source, if anyone else is willing to get involved I would much appreciate it, otherwise it looks like I have a painful journey of building ahead of me...

Member

pelson commented Aug 30, 2012

I have been developing this with python2.7, but it appears that this is not compatible with python3.2 nor python2.6. I don't have local installations of either and would have to build both from source, if anyone else is willing to get involved I would much appreciate it, otherwise it looks like I have a painful journey of building ahead of me...

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Aug 30, 2012

Contributor

I have a few python 3 fixes for you in this commit:
pwuertz/matplotlib@0e7c6e3

Successfully pickled and unpickled a simple figure with python3 using that patch on top.

Contributor

pwuertz commented Aug 30, 2012

I have a few python 3 fixes for you in this commit:
pwuertz/matplotlib@0e7c6e3

Successfully pickled and unpickled a simple figure with python3 using that patch on top.

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Aug 30, 2012

Contributor

The figure from your test_complete also pickles/unpickles fine with python3, but apparently you cannot unpickle data that has been pickled with python2...

Contributor

pwuertz commented Aug 30, 2012

The figure from your test_complete also pickles/unpickles fine with python3, but apparently you cannot unpickle data that has been pickled with python2...

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Aug 30, 2012

Member

@pwuertz: awesome! I've cherry picked that commit into this PR.

Member

pelson commented Aug 30, 2012

@pwuertz: awesome! I've cherry picked that commit into this PR.

@pwuertz

This comment has been minimized.

Show comment Hide comment
@pwuertz

pwuertz Aug 30, 2012

Contributor

Ah, hold on. In python3 you need a Byte buffer instead of a String buffer for pickle. This commit here makes the test module work.
pwuertz/matplotlib@8753aaa

I don't know if the code is now further away from being python2.6 compatible.

Also, our python editors seem to have a fight over a few whitespaces :)

Contributor

pwuertz commented Aug 30, 2012

Ah, hold on. In python3 you need a Byte buffer instead of a String buffer for pickle. This commit here makes the test module work.
pwuertz/matplotlib@8753aaa

I don't know if the code is now further away from being python2.6 compatible.

Also, our python editors seem to have a fight over a few whitespaces :)

@mdboom

This comment has been minimized.

Show comment Hide comment
@mdboom

mdboom Aug 30, 2012

Owner

As for testing on different versions of Python -- hopefully the Travis bot will start to be helpful with that. We have a number of failures on master at the moment, so it makes everything look like it's failing -- but manually expecting the results should hopefully be enough to track down whether a given pull request introduces any new failures.

Owner

mdboom commented Aug 30, 2012

As for testing on different versions of Python -- hopefully the Travis bot will start to be helpful with that. We have a number of failures on master at the moment, so it makes everything look like it's failing -- but manually expecting the results should hopefully be enough to track down whether a given pull request introduces any new failures.

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Aug 30, 2012

This pull request fails (merged f4245bb into cf7618c).

This pull request fails (merged f4245bb into cf7618c).

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Aug 30, 2012

Member

I'm confused. How do I use travis bot? The web interface sure looks pretty, but I do not understand where to see what tests are failing where. Is there documentation?

Member

dmcdougall commented Aug 30, 2012

I'm confused. How do I use travis bot? The web interface sure looks pretty, but I do not understand where to see what tests are failing where. Is there documentation?

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Aug 30, 2012

This pull request fails (merged 1e08190 into cf7618c).

This pull request fails (merged 1e08190 into cf7618c).

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Sep 1, 2012

Member

@pwuertz: Thanks for the added commit. I will need to do a bit of work to make that code python2 compatible, but that's fine. Mightn't be until Monday now though.

Member

pelson commented Sep 1, 2012

@pwuertz: Thanks for the added commit. I will need to do a bit of work to make that code python2 compatible, but that's fine. Mightn't be until Monday now though.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Sep 1, 2012

Member

Given this is quite a significant code change I would be eager to merge this and then fix the python3 support in a subsequent PR. Some of the benefits of doing this:

  • This PR will conflict and expire quickly. Rebasing is painful and means that a full review of all changes is needed each time.
  • There are other PRs in need of some of the changes. Particularly #1125.

The biggest drawback of doing this:

  • Currently, the pickle test will fail on python3 (and possibly python2.6)

Is anyone willing to merge this?

Member

pelson commented Sep 1, 2012

Given this is quite a significant code change I would be eager to merge this and then fix the python3 support in a subsequent PR. Some of the benefits of doing this:

  • This PR will conflict and expire quickly. Rebasing is painful and means that a full review of all changes is needed each time.
  • There are other PRs in need of some of the changes. Particularly #1125.

The biggest drawback of doing this:

  • Currently, the pickle test will fail on python3 (and possibly python2.6)

Is anyone willing to merge this?

@@ -608,7 +609,14 @@ def new_figure_manager(num, *args, **kwargs):
# main-level app (egg backend_gtk, backend_gtkagg) for pylab
FigureClass = kwargs.pop('FigureClass', Figure)
thisFig = FigureClass(*args, **kwargs)
- canvas = FigureCanvasPgf(thisFig)
+ return new_figure_manager_given_figure(thisFig)

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Sep 1, 2012

Owner

Don't you need a "num" argument here, to match the signature below?

@efiring

efiring Sep 1, 2012

Owner

Don't you need a "num" argument here, to match the signature below?

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Sep 1, 2012

Member

Yes. Thanks Eric.

@pelson

pelson Sep 1, 2012

Member

Yes. Thanks Eric.

efiring added a commit that referenced this pull request Sep 1, 2012

Merge pull request #1175 from pelson/pickle2
Pickling support added. Various whitespace fixes as a result of reading *lots* of code.

@efiring efiring merged commit 4c1e36d into matplotlib:master Sep 1, 2012

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Sep 1, 2012

Owner

With some trepidation, but in the interest of keeping things moving, I went ahead with the merge. I suspect that will wreak havoc with many other pull requests, but being small, they will not be difficult to fix.

Owner

efiring commented Sep 1, 2012

With some trepidation, but in the interest of keeping things moving, I went ahead with the merge. I suspect that will wreak havoc with many other pull requests, but being small, they will not be difficult to fix.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Sep 1, 2012

Member

Woohoo! This has me a little too excited. Good work!

Member

dmcdougall commented Sep 1, 2012

Woohoo! This has me a little too excited. Good work!

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Sep 2, 2012

Member

@efiring: Thanks for doing that: I think it is a good decision. It will certainly save me time in having to rebase (and other re-read), which I intend to invest to other mpl related things. As you can see, I have created a new PR with the feedback from this PR applied.

@dmcdougall : Thanks for the back pat. Its good to know that these features are appreciated. Do you have a particular use in mind for pickle support or is it just a general purpose "nice to have"?

Member

pelson commented Sep 2, 2012

@efiring: Thanks for doing that: I think it is a good decision. It will certainly save me time in having to rebase (and other re-read), which I intend to invest to other mpl related things. As you can see, I have created a new PR with the feedback from this PR applied.

@dmcdougall : Thanks for the back pat. Its good to know that these features are appreciated. Do you have a particular use in mind for pickle support or is it just a general purpose "nice to have"?

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Sep 2, 2012

Member

@pelson Well, it is 'nice to have'. Though, in particular, it would save me a lot of time when I write a script that generates 8 figures from several 2GB data files and then a co-author tells me that there are too many yticks.

Member

dmcdougall commented Sep 2, 2012

@pelson Well, it is 'nice to have'. Though, in particular, it would save me a lot of time when I write a script that generates 8 figures from several 2GB data files and then a co-author tells me that there are too many yticks.

@pelson

This comment has been minimized.

Show comment Hide comment
@pelson

pelson Sep 2, 2012

Member

Though, in particular, it would save me a lot of time when I write a script that generates 8 figures from several 2GB data files and then a co-author tells me that there are too many yticks.

Yes. I like it. ;-) Your right: it is the general purpose - little things which will save a little bit of time time - benefits that I see this delivering most of all.

Member

pelson commented Sep 2, 2012

Though, in particular, it would save me a lot of time when I write a script that generates 8 figures from several 2GB data files and then a co-author tells me that there are too many yticks.

Yes. I like it. ;-) Your right: it is the general purpose - little things which will save a little bit of time time - benefits that I see this delivering most of all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment