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

symlog-scale: Remove asssert linscale >= 1. #3053

Merged
merged 1 commit into from May 22, 2014
Merged

symlog-scale: Remove asssert linscale >= 1. #3053

merged 1 commit into from May 22, 2014

Conversation

Tillsten
Copy link
Contributor

@Tillsten Tillsten commented May 9, 2014

I can't see why the assert was needed and it does excatly what i would expect from it.

@Tillsten
Copy link
Contributor Author

Tillsten commented May 9, 2014

Hmm, i dont think the travis-fail is from this change.
Would fix #2288.
@mdboom wrote the orignal code.

@tacaswell
Copy link
Member

There is a known intermittent failure on travis on 3.2 for font priority.

@tacaswell tacaswell added this to the v1.4.0 milestone May 9, 2014
@tacaswell
Copy link
Member

@mdboom Can you take a look at this? My only worry is that there was a good reason for >=1 that we are not seeing.

@tacaswell
Copy link
Member

@Tillsten Could you please add a CHANGELOG entry?

I can't see why the assert was needed and it does excatly what i would expect from it.
@Tillsten
Copy link
Contributor Author

done, failure due to unrelated font issues.

@Tillsten
Copy link
Contributor Author

If @mdboom doesen't have time, can we merge it anyways? I see no broken behavior
with linscale smaller than 1, it does excatly what one would expect, from docstring there is
nothing indicating a value < 1 should be dissallowed and tests still pass.

@efiring efiring merged commit aa95f93 into matplotlib:master May 22, 2014
@efiring
Copy link
Member

efiring commented May 22, 2014

@tacaswell, This looked reasonable, so I did the manual changelog merge and pushed. It looks like I should have done it all in one merge operation instead of 2; I haven't done this for a while.

@Tillsten Tillsten deleted the patch-1 branch May 22, 2014 19:11
@Tillsten
Copy link
Contributor Author

Thanks!

@tacaswell
Copy link
Member

merged to master: 4b1bd63 and conflicts resolved: 9d72862.

I have been doing the manual merges in one step (merge user/pr_branch into local master -> push to matplotlib/master), but its not a huge deal. One could argue doing it in two steps is better as then the merge to master is conflict-free.

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