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

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

Merged
merged 1 commit into from Dec 3, 2012

Conversation

montefra
Copy link
Contributor

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
Copy link
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.

@dmcdougall
Copy link
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?

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

ax.grid(True)

ax.grid(gridon)
Copy link
Member

Choose a reason for hiding this comment

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

I think you lost some indentation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

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

@montefra
Copy link
Contributor Author

@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
Copy link
Member

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

Edit: grammar.

@montefra
Copy link
Contributor Author

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

@dmcdougall
Copy link
Member

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

@montefra
Copy link
Contributor Author

@dmcdougall done without making a mess.

dmcdougall added a commit that referenced this pull request Dec 3, 2012
pyplot.plotfile. gridon option added with default from rcParam.
@dmcdougall dmcdougall merged commit f1dbe0e into matplotlib:master Dec 3, 2012
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.

None yet

4 participants