pyplot.plotfile. gridon option added with default from rcParam. #1297

Merged
merged 1 commit into from Dec 3, 2012

Conversation

Projects
None yet
4 participants
@montefra
Contributor

montefra commented Sep 22, 2012

This should be a clean pull request of issue: #1205

Recap: the original plotfile function draw the axes grids at each call. This pull request implements a
keywords to enable or disable the grid. The default is taken from rc.Params['axes.gris'].
This keyword is deprecated and will be deleted from future releases

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Sep 22, 2012

Member

Thanks for that, @montefra. If you ever need to update this pull request you can do the following:

  1. Make sure you are currently on the plotfile_grid branch:

git checkout plotfile_grid

  1. Make any changes you want:

git add ...
git commit

  1. Push the branch:

git push origin plotfile_grid

The updates will display here automagically.

Member

dmcdougall commented Sep 22, 2012

Thanks for that, @montefra. If you ever need to update this pull request you can do the following:

  1. Make sure you are currently on the plotfile_grid branch:

git checkout plotfile_grid

  1. Make any changes you want:

git add ...
git commit

  1. Push the branch:

git push origin plotfile_grid

The updates will display here automagically.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Sep 22, 2012

Member

@efiring You had concerns about this change hurting users. Would an entry in api_changes.rst address those concerns, or would a warning be more appropriate?

Member

dmcdougall commented Sep 22, 2012

@efiring You had concerns about this change hurting users. Would an entry in api_changes.rst address those concerns, or would a warning be more appropriate?

@dmcdougall

View changes

lib/matplotlib/pyplot.py
@@ -2189,8 +2192,7 @@ def getname_val(identifier):
elif i==1:
ax = fig.add_subplot(1,1,1)
- ax.grid(True)
-
+ ax.grid(gridon)

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Sep 22, 2012

Member

I think you lost some indentation here.

@dmcdougall

dmcdougall Sep 22, 2012

Member

I think you lost some indentation here.

This comment has been minimized.

Show comment Hide comment
@montefra

montefra Sep 23, 2012

Contributor

I thought that too. But if you go to my repository (https://github.com/montefra/matplotlib/blob/plotfile_grid/lib/matplotlib/pyplot.py) ax.grid(gridon) has the right indentation.
Can be a problem with tabs. I use vim with some selfindent and it seems that it converts spaces to tabs when they are many.
I've tested the code before submitting and didn't get any indent error. I can substitute the tab with spaces and recommit if it's a problem.

@montefra

montefra Sep 23, 2012

Contributor

I thought that too. But if you go to my repository (https://github.com/montefra/matplotlib/blob/plotfile_grid/lib/matplotlib/pyplot.py) ax.grid(gridon) has the right indentation.
Can be a problem with tabs. I use vim with some selfindent and it seems that it converts spaces to tabs when they are many.
I've tested the code before submitting and didn't get any indent error. I can substitute the tab with spaces and recommit if it's a problem.

This comment has been minimized.

Show comment Hide comment
@WeatherGod

WeatherGod Sep 23, 2012

Member

On Sunday, September 23, 2012, Francesco Montesano wrote:

In lib/matplotlib/pyplot.py:

@@ -2189,8 +2192,7 @@ def getname_val(identifier):
elif i==1:
ax = fig.add_subplot(1,1,1)

- ax.grid(True)

  •   ax.grid(gridon)
    

I thought that too. But then if you go to my repository (
https://github.com/montefra/matplotlib/blob/plotfile_grid/lib/matplotlib/pyplot.py)
ax.grid(gridon) has the right indentation.
Can be a problem with tabs. I use vim with some selfindent and it seems
that it converts spaces to tabs when they are many.
I've tested the code before submitting and didn't get any indent error. I
can substitute the tab with spaces and recommit if it's a problem.

Do a google search for "python vimrc options", and you will find an
appropriate set of options to use. They will work properly even with self
indent.

@WeatherGod

WeatherGod Sep 23, 2012

Member

On Sunday, September 23, 2012, Francesco Montesano wrote:

In lib/matplotlib/pyplot.py:

@@ -2189,8 +2192,7 @@ def getname_val(identifier):
elif i==1:
ax = fig.add_subplot(1,1,1)

- ax.grid(True)

  •   ax.grid(gridon)
    

I thought that too. But then if you go to my repository (
https://github.com/montefra/matplotlib/blob/plotfile_grid/lib/matplotlib/pyplot.py)
ax.grid(gridon) has the right indentation.
Can be a problem with tabs. I use vim with some selfindent and it seems
that it converts spaces to tabs when they are many.
I've tested the code before submitting and didn't get any indent error. I
can substitute the tab with spaces and recommit if it's a problem.

Do a google search for "python vimrc options", and you will find an
appropriate set of options to use. They will work properly even with self
indent.

@efiring

This comment has been minimized.

Show comment Hide comment
@efiring

efiring Sep 22, 2012

Owner

This is very confusing, despite being a tiny change. From reading the predecessor, #1205, I gathered that the decision was to just make the jump: leave out the new kwarg, and use the rcParams value internally, for consistency with other plotting. However, this PR has the kwarg back in. Furthermore, it is used in a particularly confusing way, such that only the rcParams value at the time the function is defined takes effect; changing the rcParams value before the function is called will not affect the function:

In [1]: x = dict(a=1)
In [2]: def f(xx=x['a']):
   ...:     print xx
   ...:     
In [3]: f()
1
In [4]: x['a'] = 2
In [5]: f()
1

It looks like the best way to handle this is going to be to make a clean break--knowing it may cause some presumably minor breakage (changing plot appearance, not information content) in user code. So I think we need to leave the kwarg out, and simply use the rcParams value internally. This requires a note in api_changes.

If @mdboom or others would prefer not to make this api change (which is almost a bug fix; it is repairing a design flaw in plotfile) for v1.2, then it (meaning not the present PR, but the tiny change outlined above, as a clean single changeset to whichever version is selected as the target) can be targeted to master instead. It's not urgent.

Owner

efiring commented Sep 22, 2012

This is very confusing, despite being a tiny change. From reading the predecessor, #1205, I gathered that the decision was to just make the jump: leave out the new kwarg, and use the rcParams value internally, for consistency with other plotting. However, this PR has the kwarg back in. Furthermore, it is used in a particularly confusing way, such that only the rcParams value at the time the function is defined takes effect; changing the rcParams value before the function is called will not affect the function:

In [1]: x = dict(a=1)
In [2]: def f(xx=x['a']):
   ...:     print xx
   ...:     
In [3]: f()
1
In [4]: x['a'] = 2
In [5]: f()
1

It looks like the best way to handle this is going to be to make a clean break--knowing it may cause some presumably minor breakage (changing plot appearance, not information content) in user code. So I think we need to leave the kwarg out, and simply use the rcParams value internally. This requires a note in api_changes.

If @mdboom or others would prefer not to make this api change (which is almost a bug fix; it is repairing a design flaw in plotfile) for v1.2, then it (meaning not the present PR, but the tiny change outlined above, as a clean single changeset to whichever version is selected as the target) can be targeted to master instead. It's not urgent.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 19, 2012

Member

@montefra Could you update this to be in line with @efiring's suggestion?

Member

dmcdougall commented Nov 19, 2012

@montefra Could you update this to be in line with @efiring's suggestion?

@montefra

This comment has been minimized.

Show comment Hide comment
@montefra

montefra Nov 19, 2012

Contributor

@dmcdougall: gridon removed. I also think that is neater if plotfile works as all the other pyplot functions
@efiring 👍 I didn't know that function arguments are evaluated a definition time, not at execution time.

Contributor

montefra commented Nov 19, 2012

@dmcdougall: gridon removed. I also think that is neater if plotfile works as all the other pyplot functions
@efiring 👍 I didn't know that function arguments are evaluated a definition time, not at execution time.

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 19, 2012

Member

@montefra You'll also need to add a little note in api_changes.rst warning users of the new behaviour.

Edit: grammar.

Member

dmcdougall commented Nov 19, 2012

@montefra You'll also need to add a little note in api_changes.rst warning users of the new behaviour.

Edit: grammar.

@montefra

This comment has been minimized.

Show comment Hide comment
@montefra

montefra Nov 20, 2012

Contributor

@dmcdougall done. I hope the the English is good enough

Contributor

montefra commented Nov 20, 2012

@dmcdougall done. I hope the the English is good enough

@dmcdougall

This comment has been minimized.

Show comment Hide comment
@dmcdougall

dmcdougall Nov 20, 2012

Member

@montefra Thanks. Can you squash all this down to a single commit?

Member

dmcdougall commented Nov 20, 2012

@montefra Thanks. Can you squash all this down to a single commit?

@montefra

This comment has been minimized.

Show comment Hide comment
@montefra

montefra Nov 21, 2012

Contributor

@dmcdougall done without making a mess.

Contributor

montefra commented Nov 21, 2012

@dmcdougall done without making a mess.

dmcdougall added a commit that referenced this pull request Dec 3, 2012

Merge pull request #1297 from montefra/plotfile_grid
pyplot.plotfile. gridon option added with default from rcParam.

@dmcdougall dmcdougall merged commit f1dbe0e into matplotlib:master Dec 3, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment