Skip to content

fixed issue #5456 #6167

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

Closed
wants to merge 3 commits into from
Closed

fixed issue #5456 #6167

wants to merge 3 commits into from

Conversation

CrazyKsv
Copy link

bugfix for bug-#5456

This is a simple fix by adding a value check.
If you need testing cases, please let me know

In this scenario, bottom and top are the margins(white space) that are set between the figure and the top/bottom of the image. If the y-axis label is set to a long string, it causes the calculations of these margins in tight_layout() to be greater than their max length 0.5, which then raises the ValueError as mentioned before. The size of the figure should be in the range 0.0-1.0 for both x and y lengths. If one of the margins exceed 0.5, then the image produced would have too much empty space on one half. If margins top and bottom exceed 0.5, then the figure will be left with no space to be displayed, which causes the program to crash.

Also, We fixed the same problem that appear in horizontal label.
Through investigation, it was discovered that a similar bug happens when the label assigned to the x-axis exceeds the allotted space for the figure. Namely, the error that occurs is ValueError: left cannot be >= right and crashes the python program. We have implemented the same fix to apply for the x-axis as well.

Our initial commit for bug #5456 passed the check..
For the commit and revert of bug #4414, please ignore the commit of 2a02dfc and 7902d5d
Those will be committed by my teammates.

@mdboom mdboom changed the title fixed issue #5456 fixed issue #5456 Mar 16, 2016
@CrazyKsv
Copy link
Author

Our initial commit for bug #5456 passed the check..
For the commit and revert of bug #4414, please ignore the commit of 2a02dfc and 7902d5d
Those will be committed by my teammates.

@QuLogic
Copy link
Member

QuLogic commented Mar 17, 2016

I don't know what you mean by ignore the commits; if you don't want them to be committed, you need to remove them from this PR.

@CrazyKsv
Copy link
Author

@QuLogic I accidentally commit the bug fixed for #4414. And had just reverted the changes for #4414. Is that ok or I need to do Anothet PR for #5456 ?

@tacaswell
Copy link
Member

The issue is that PRs track branchs not commits (so you can just keep adding commits to a PR). This is typically why you should always work on feature branches (and not an 'master'). I suggest you do the following:

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Mar 17, 2016
# bugfix for bug-#5456
# simple check for the error
if margin_left > 0.5:
margin_left = 0.05
Copy link
Member

Choose a reason for hiding this comment

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

Why these magic numbers? It might be more reliable to do a check for the actual fail condition (margin_left > 1 - margin_right) at the end and fix it there once you know all of the margins.

@tacaswell
Copy link
Member

There might also be a case for special-casing overly long x and y labels and (maybe) auto-wrapping them.

con:

  • dirty special cased magic

pro:

  • if users are using tight_layout that is the sort of magic they might expect.

@tacaswell
Copy link
Member

Sorry, ignore some of my comments re new PRs, you can probably sort out the order I am reading things in 🐑

@anntzer
Copy link
Contributor

anntzer commented May 13, 2017

I am closing this as I believe we should instead have a more general solution which raises whenever the padding/spacing parameters make tight-layouting impossible (#8062, #8201, #8202) (or handles correctly such cases).
Feel free to ask for a reopen if you disagree.

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.

5 participants