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

Fixed handling of bar(.., bottom=None, log=True) #1889

Merged
merged 2 commits into from May 21, 2013

Conversation

tacaswell
Copy link
Member

re-added lines to make sure that bottom and left are not None in the case of log=True. If this is not done, there will be type exceptions.

Issue #1882

@tacaswell
Copy link
Member Author

build errors are inkscape installation errors.

@dmcdougall
Copy link
Member

I fixed the install errors in 01a4bed.

Sorry to go behind protocol on that commit; I used the GitHub editor and I thought it would make a pull request out of it rather than commit directly to the repository.

the case of `log=True`.  If this is not done, there will be type
exceptions.

Issue matplotlib#1882
@tacaswell
Copy link
Member Author

@dmcdougall ok, rebased.

@neggert
Copy link
Contributor

neggert commented Apr 9, 2013

I think I introduced this bug. Can we get a test so it doesn't happen again?

@tacaswell
Copy link
Member Author

@neggert Can you write that test (I am super swamped, have a committee meeting in a week)? I don't think we need an image test for this, just make sure all reasonable arguments don't blow up.

Can you also take a look at the other issues @y-p has brought up in the original issue thread?

@dmcdougall do you want me to rebase this again to get all of the svg tests to run?

@neggert
Copy link
Contributor

neggert commented Apr 9, 2013

I can do the test, but it might take me a few days to get to it. I propose to merge this now. I'll make a separate PR with the test when I get to it.

@@ -4797,6 +4797,7 @@ def make_iterable(x):
if _bottom is None:
if self.get_yscale() == 'log':
adjust_ylim = True
bottom = [1e-100]
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to move this outside of the if statement and just set it to [0]. That was the reason for making the change (nonposy=True) that got this all started. That way it will behave well even if someone wants to work with very small numbers.

@tacaswell
Copy link
Member Author

@neggert Done. I suspected that would be the case, but went with what seemed to be the minimal change (which was replacing those lines)

@tacaswell
Copy link
Member Author

@neggert Do you agree with how this is fixed now?

@neggert
Copy link
Contributor

neggert commented Apr 15, 2013

Yup! Thanks. 

Sent from Mailbox for iPhone

On Mon, Apr 15, 2013 at 10:21 AM, Thomas A Caswell
notifications@github.com wrote:

@neggert Do you agree with how this is fixed now?

Reply to this email directly or view it on GitHub:
#1889 (comment)

@tacaswell
Copy link
Member Author

@mdboom Can this be merged?

mdboom added a commit that referenced this pull request May 21, 2013
Fixed handling of `bar(.., bottom=None, log=True)`
@mdboom mdboom merged commit 16be971 into matplotlib:v1.2.x May 21, 2013
@tacaswell tacaswell deleted the log_bar_regression branch May 21, 2013 20:17
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