Skip to content

bar plot: in 2.0.0 bars not as given in the description, ie. first arg is not "left" but "center" #7954

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
t1nux opened this issue Jan 26, 2017 · 13 comments · Fixed by #9110
Closed
Labels
API: default changes Documentation Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Milestone

Comments

@t1nux
Copy link

t1nux commented Jan 26, 2017

Bug report

Bug summary

Using bar, the first input argument is not used for the left edge of the bar, but rather the center, as if the kwarg align='center' was used. However, the desired behavior is obtained in both cases when using align = 'center' and align = 'edge'. Just when align is not specified, it should be 'edge' aligned which it is not.

Code for reproduction

import matplotlib.pyplot as plt
plt.bar([1], [1])
plt.show()

Actual outcome
figure_1
Expected outcome
figure_2

In matplotlib 1.5.3 I got the expected behavior.

Matplotlib version
matplotlib 2.0.0
python 3.6.0
both installed from the Arch Linux repository.

@phobson phobson added this to the 2.0.1 (next bug fix release) milestone Jan 26, 2017
@phobson
Copy link
Member

phobson commented Jan 26, 2017

My hunch is that this a documentation/naming convention oversight in the wake of the changes to the defaults.

IIRC, it was a bad user experience to align in the bars along the left edge (the previous default). So at some point, we added the align kwarg so that the left parameter could actually specify the center of the bars. At that point, we should have renamed left to x, but probably could not for backwards compatibility reasons.

Then with v2 we changed the default value of align to "center", but still did not update the symbol left to x.

I'm not sure what the best path forward is.

@tacaswell
Copy link
Member

This is definitely intended behavior. The docstring on left should include a warning / comment on this.

Unfortunately we can not re-name the first positional argument without breaking API, I think that is a wart we are stuck with.

@t1nux
Copy link
Author

t1nux commented Jan 27, 2017

I agree that left align was a poor user experience and I welcome the change to center being the default. I was just surprised to see the change in one of my scripts after updating to 2.0.0, in particular since the description did not reflect the obtained behavior.

I would suggest, though, to also make the same change to step, which, in 1.5.3, could be used somewhat interchangeably to bar, if none of the alignment options (align for bar and where for step) keywords were used. This is not the case anymore.

@jason-s
Copy link

jason-s commented Mar 29, 2017

IIRC, it was a bad user experience to align in the bars along the left edge

Except that aligning bars along the center when using an axis with xscale='log' is a bit of a pain to deal with; you need to take the geometric mean of successive pairs edges.

Oh, never mind, you need to use arithmetic mean with xscale='log'. Gah. I don't really like this change (it's very weird when you're specifying widths so that you can get bars that butt up right next to each other) but at least I can use align='edge' and get the same behavior as before.

@pierre-haessig
Copy link
Contributor

I also got hit by the doctring inconsistency (not by the change of default value, because I rarely use bar).

@tacaswell is it really impossible to rename the first argument from left to x and advertise this as delayed change part of the 2.0 release. Of course a solution is to add a warning further down in the docstring. It would be a good bandage, but it wouldn't fix for this terrible API inconsistency.

@pierre-haessig
Copy link
Contributor

also as a different, but related, issue on the documentation of bar function: there is a data keyword argument in the signature which, I believe, is not documented.

@WeatherGod
Copy link
Member

WeatherGod commented Apr 10, 2017 via email

@QuLogic QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017
@skjaeve
Copy link

skjaeve commented Jul 17, 2017

@tacaswell

Unfortunately we can not re-name the first positional argument without breaking API, I think that is a wart we are stuck with.

Isn't the API already broken if the meaning of a positional argument was changed?

@WeatherGod
Copy link
Member

WeatherGod commented Jul 17, 2017 via email

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.0.3 (next bug fix release) Jul 18, 2017
@tacaswell
Copy link
Member

it is 'broken' in the sense that it is confusing not 'broken' in the sense that code will raise exceptions.

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 18, 2017
@anntzer
Copy link
Contributor

anntzer commented Jul 18, 2017

Still advertising #7966 as a way to fix the signature...

@tacaswell
Copy link
Member

The consensus on the phone call today is:

  • change to *args, **kwargs
  • use Signature.bind to try the current signature and (x, height, width=0.8, bottom=None, **kwargs) and (left, hegiht, width=0.8, bottom=None, **kwargs)
  • document the first version in the docstring (and expose out if possible).

@anntzer
Copy link
Contributor

anntzer commented Jul 25, 2017

Actually, upon further thought, we don't need to create signature objects; we can just do something like #6404 (comment). The advantage of using signature objects when using the #7966 decorator approach is that we can simulate positional-only args or even on Py2 keyword-only args, but if we're not using the decorator approach we may as well let the interpreter do the argument binding for us. So something like

def bar(*args, **kwargs):
    matchers = [
        (lambda x, height, width, bottom, **kwargs:
         (x, height, width, bottom, kwargs)),
        (lambda left, height, width, bottom, **kwargs:
         (left, height, width, bottom, kwargs))]
    for matcher in matchers:
        try:
            x, height, width, bottom, kwargs = matcher(*args, **kwargs)
        except TypeError:
            # This can only come from a no-match as there is no other logic in
            # the matchers.
            continue
        else:
            break
    else:
        raise TypeError("...")  # We can even re-use the TypeError from the first match.
    ...

tacaswell added a commit to tacaswell/matplotlib that referenced this issue Aug 27, 2017
Change the documented first position argument to x and y for
bar and barh respectively but still support passing in left and bottom
as keyword arguments.

closes matplotlib#7954
closes matplotlib#8327
closes matplotlib#8527
closes matplotlib#8882
tacaswell added a commit to tacaswell/matplotlib that referenced this issue Aug 27, 2017
Change the documented first position argument to x and y for
bar and barh respectively but still support passing in left and bottom
as keyword arguments.

closes matplotlib#7954
closes matplotlib#8327
closes matplotlib#8527
closes matplotlib#8882
tacaswell added a commit to tacaswell/matplotlib that referenced this issue Aug 27, 2017
Change the documented first position argument to x and y for
bar and barh respectively but still support passing in left and bottom
as keyword arguments.

closes matplotlib#7954
closes matplotlib#8327
closes matplotlib#8527
closes matplotlib#8882
@tacaswell tacaswell mentioned this issue Aug 27, 2017
4 tasks
tacaswell added a commit to tacaswell/matplotlib that referenced this issue Aug 28, 2017
Change the documented first position argument to x and y for
bar and barh respectively but still support passing in left and bottom
as keyword arguments.

closes matplotlib#7954
closes matplotlib#8327
closes matplotlib#8527
closes matplotlib#8882
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: default changes Documentation Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants