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

Step linestyle #1802

Merged
merged 7 commits into from Mar 23, 2013
Merged

Step linestyle #1802

merged 7 commits into from Mar 23, 2013

Conversation

tacaswell
Copy link
Member

Looked at this because of http://stackoverflow.com/questions/15188005/linestyle-in-matplotlib-step-function/15191183#15191183

The step function is just a light wrapper of plot. Added a step to append the user specified linestyle onto the argument passed to plot instead of silently ignoring it.

Includes a unit test (there is currently no other test-coverage of step).

Based this against master because I am not sure if this is really a bug, or if I am changing an intentional interface.

@@ -1187,3 +1187,30 @@ def test_eb_line_zorder():
if __name__=='__main__':
Copy link
Member

Choose a reason for hiding this comment

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

You need to remove this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what you mean here. My editor removes white space at the end of lines when it saves so this looks like an un-intentional pep8 fix to me.

Copy link
Member

Choose a reason for hiding this comment

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

the code if __name__ == "__main__" is duplicated in this file. It should be at the bottom of the file only, and not in the middle of the module. As you added it again after your code, you need to remove line 1187 to line 1189.

Copy link
Member Author

Choose a reason for hiding this comment

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

right, I was looking at the wrong bit of code.

@tacaswell
Copy link
Member Author

Sorry about the style issues.

@tacaswell
Copy link
Member Author

The 2.6 failure is related to pip and pyparsing

@mdboom
Copy link
Member

mdboom commented Mar 5, 2013

The build error looks to just be a networking timeout. Typical for travis.

@@ -4793,7 +4793,8 @@ def step(self, x, y, *args, **kwargs):
if where not in ('pre', 'post', 'mid'):
raise ValueError("'where' argument to step must be "
"'pre', 'post' or 'mid'")
kwargs['linestyle'] = 'steps-' + where
ln_sty = kwargs.pop('linestyle', '')
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 this variable would benefit from a more explicite name.

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be a more explicit name? (I think there is some level of judgement here as I thought that was already a pretty explicit name). I have no problem changing it, I would just like to only change it once ;)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like linestyle if it is not already taken :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I am (probably unjustly) paranoid about shadowing/trampling existing variables. How about usr_linestyle which makes me happy and makes it clear that this is the part of the linestyle that the user passed in, not the total linestyle that will be passed to plot

Copy link
Member

Choose a reason for hiding this comment

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

sounds good !

@NelleV
Copy link
Member

NelleV commented Mar 15, 2013

Apart from my last tiny nitpick, this looks good to me: 👍

@tacaswell
Copy link
Member Author

@NelleV done

@tacaswell
Copy link
Member Author

I am not sure why it is failing, I will look into this later tonight.

@tacaswell
Copy link
Member Author

The failure is due to what looks like a few pixel shift of the entire figure in the png output that I suspect is from the change is c64d0c25668b08cda4c19bb0c3c7ebfa56e47afe (based on the large number of image updates that go along with it).

At any rate, rebased and replaced the offending test image.

@tacaswell
Copy link
Member Author

and if I had been thinking I would have also squashed some of those commits out of existence while I was re-writing history, but oh well.

@mdboom
Copy link
Member

mdboom commented Mar 18, 2013

Nice. The only thing to add is probably a changelog entry to users now they can now do this.

@pelson
Copy link
Member

pelson commented Mar 19, 2013

👍 Nice work.

dmcdougall added a commit that referenced this pull request Mar 23, 2013
@dmcdougall dmcdougall merged commit a844fbb into matplotlib:master Mar 23, 2013
@tacaswell tacaswell deleted the step_linestyle branch March 24, 2013 19:28
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

5 participants