Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Api bar signature#9110
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Api bar signature#9110
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lib/matplotlib/axes/_axes.py Outdated
| """ | ||
| kwargs = cbook.normalize_kwargs(kwargs, mpatches._patch_alias_map) | ||
| matchers = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why not just
matchers = [ lambda x, height, width=0.8, bottom=None, **kwargs: (False, x, height, width, bottom, kwargs), lambda left, height, width=0.8, bottom=None, **kwargs: (False, left, height, width, bottom, kwargs),]?
or did I miss a case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ah, that is better 🐑
lib/matplotlib/axes/_axes.py Outdated
| Make a bar plot with rectangles bounded by: | ||
| `left`, `left` + `width`, `bottom`, `bottom` + `height` | ||
| `x` - width/2, `x` + `width`/2, `bottom`, `bottom` + `height` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These should be changed to use double backquotes around each complete expression.
lib/matplotlib/axes/_axes.py Outdated
| if "label" not in error_kw: | ||
| error_kw["label"] = '_nolegend_' | ||
| errorbar = self.errorbar(x, y, | ||
| errorbar = self.errorbar(ex, ey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
just above (line 2183) can be switched to use setdefault.
| barh(bottom, width, *, align='center', **kwargs) | ||
| despite behaving as the center in both cases. The methods now take ``*args, **kwargs`` | ||
| is input and are documented to have the primary signatures of :: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no space before ::
lib/matplotlib/axes/_axes.py Outdated
| else: | ||
| break | ||
| else: | ||
| raise TypeError("Missing two required positional arguments: 'x'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you save the first exception and just reraise it here? this will make the message correct in more cases (e.g. the user passes height as a kwarg but not x).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The first one won't be right, it is the 4th one we want, but if you put the 4th one first it will hit and then we would have to handle the kwargs for the two optional ones seperatly (which might be better).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nm
I think I addressed all of@anntzer 's comments, tests are in-progress. |
lib/matplotlib/axes/_axes.py Outdated
| "color", "edgecolor", "linewidth", | ||
| "tick_label", "xerr", "yerr", | ||
| "ecolor"], | ||
| label_namer=None) | ||
| label_namer=None, | ||
| positional_parameter_names=['x', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why do we need this? (I'ml not too familiar with the exact semantics of _preprocess_data, but at leasty looks a bit fishy in the argument list here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the 'y' is junk from a second rename that I ended up walking back. The purpose of this list is to tell the pre-processor the name of input hidden in the '*args' so it know which entries to try and look up indata, however in this case we want to do all of them so there is a simpler way.
lib/matplotlib/axes/_axes.py Outdated
| # size width andbottom according to length ofleft | ||
| if_bottom is None: | ||
| # size width andy according to length ofx | ||
| if_y is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we just default bottom to 0 instead of None? (looks so, except possibly for log scale?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we would have to keep this logic anyway as users can be passingNone in explicitly. Inclined to let that stay as-is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can we at least make the change go before the call to np.broadcast_arrays, so that you don't have to call zeros_like below?
(I would also deprecate allowing passing None yada yada)
Change the documented first position argument to x and y forbar and barh respectively but still support passing in left and bottomas keyword arguments.closesmatplotlib#7954closesmatplotlib#8327closesmatplotlib#8527closesmatplotlib#8882
This change is frommatplotlib#8993
CI failing with some docstring markup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A few minor things, but otherwise it's a 👍 from me.
lib/matplotlib/axes/_axes.py Outdated
| patches = self.bar(left=left, height=height, width=width, | ||
| bottom=bottom, orientation='horizontal', **kwargs) | ||
| matchers = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A comment explaining what the lambdas are for wouldn't hurt.
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Adjusts the signature of
barandbarhcloses#7954
closes#8327
closes#8527
closes#8882
PR Checklist
I'll add specific tests later tonight, but the existing test do a pretty good job of covering these changes.