#2150 Updating patch limits if either width or height is non zero: Issue 2150 #2942
Conversation
…dth or height of a patch might be 0 (but not if both width and height are 0).
|
@tacaswell If so, I will add image tests and polish it for merging. |
|
It seems reasonable to me and all of the tests pass, however I am worried about un-intended consequences. @efiring or @mdboom should sign off on this on. If there is not some compelling reason that this logic must be an One of the fun things about a large code base is you tend to assume things are done some way 'for a reason'. I am a bit concerned by the 'merge from master' in the history. Our practice is to keep re-basing branches if the conflict with master. |
|
The comment in update_patch_limits says the rationale for "or" is that otherwise log scaling has problems. I don't know offhand whether this is still the case, and if so, whether there might be a better solution to that problem. Apart from that, the "and" proposed here seems reasonable. |
|
@sfroid This needs to be rebased (conflicts in CHANGELOG). Did you get a chance to investigate how this interacts with |
|
It seems to work fine with log-scaling. Closing because I merged the non-merge commits on a local machine and pushed to master. |
|
@tacaswell - I know it adds a small overhead, but would you mind linking to the change if you end up doing it by hand? It just makes it easier to find what has gone on where. Thanks 😄 |
|
Sorry about that, I will take care of this next time I am at a real computer. |
|
Merged in 899f11d |
Previously, we would not update the patch limits, if either width or height of the patch were 0.
Now, if either width is non zero (vertical bar graph with zero entries) or height is non zero (horizontal bar graphs), the patch limits will be updated. If both width and height are zero, the patch will be ignored.