Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Replace numeric loc by position string#11174
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jklymak commentedMay 5, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
No doubt I have 🧠 damage from using matlab for too many years but I find the numbers easier to remember than the strings (at least for the corners). Is it “top” or “upper”, “bottom” or “lower”, “Center”, “centre”, or “middle”? I’d have to look it up. If we go string-only (even in the docs) I think the strings should be more forgiving and perhaps have short forms. “Ur” “lr” etc. |
timhoffm commentedMay 5, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I won't let matlab knowledge count as an argument 😄 .
Additionally, the numbers don't even have a reasonable order: Also, what's 'right' (5)? - Seems identical to 'center right' (7). I'm -0.5 on more forgiving and short forms. While it may be easier to write, it reduces readability again and I don't want to a diversity of accepted arguments. Also keep in mind, that positioning the legend is something you do to polish your plot. So, |
An alternative is compass notation: N, E, S, W, NE, NW, etc. Easy to remember, concise. |
Compass notation is currently used for anchors but not for legend locs. So this would be an API change, which would require a separate discussion. |
Well, I don't 100% disagree with you, but... I don't think matplotlib would be in the position its in today if it hadn't hewed closely to Matlab: a) because Matlab actually is quite good in general, particularly with regards to API, particularly as they have a lot of smart people working on it for the last 30+ y. b) a lot of people come here from Matlab. So, while I don't think we should duplicate all of Matlab's API choices, its not a terrible starting spot.... I think compass notation inaddition to the existing strings is a great idea. |
... but MATLAB doesn't even document numbers as locations?https://www.mathworks.com/help/matlab/ref/legend.html#bt6r30y They use compass notation, which I agree is a decent option. |
While I do not think the number notation should vanish from the legend docstring (or other docs that use this notation), this PR seems to be concerned with the examples and tutorials only. Because such examples contain code that is to beread by people, it is useful to make it as understandable as possible. Using a clear notation helps to establish a connection between the code and the output, i.e. if the code reads |
Yes, as long as the docstring still has the locations thats fine. |
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.
I didn't check every entry to see if the loc numbers -> string exactly, but this looks good.
Well, its been a while since I last used it 😉 http://matlab.izmiran.ru/help/techdoc/ref/legend.html |
So now MATLAB docs live on shady Russian websites? :p (sorry to any Russian I'm offending 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.
👍@timhoffm feel free to either use my suggestions or merge this yourself if you don't want to
| @@ -261,11 +261,11 @@ rectangle for the size bar. | |||
| fig, ax = plt.subplots(figsize=(3, 3)) | |||
| bar0 = AnchoredSizeBar(ax.transData, 0.3, 'unfilled', loc=3, frameon=False, | |||
| size_vertical=0.05, fill_bar=False) | |||
| bar0 = AnchoredSizeBar(ax.transData, 0.3, 'unfilled', loc='lower left', | |||
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.
I wonder whether we should really change old what's new entries at all?
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.
In this case, I'm in favor of changing. 'lower left' was already available at that time and should have been preferred to the number. We don't change any semantics but make the example more readable.
| @@ -50,7 +51,7 @@ def test_legend_auto2(): | |||
| x = np.arange(100) | |||
| b1 = ax.bar(x, x, align='edge', color='m') | |||
| b2 = ax.bar(x, x[::-1], align='edge', color='g') | |||
| ax.legend([b1[0], b2[0]], ['up', 'down'], loc=0) | |||
| ax.legend([b1[0], b2[0]], ['up', 'down'], loc='best') | |||
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.
loc='best' is default, so maybe could be omitted altogether?
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.
Yes. I've removed them.
lib/matplotlib/tests/test_legend.py Outdated
| @@ -39,7 +40,7 @@ def test_legend_auto1(): | |||
| x = np.arange(100) | |||
| ax.plot(x, 50 - x, 'o', label='y=1') | |||
| ax.plot(x, x - 50, 'o', label='y=-1') | |||
| ax.legend(loc=0) | |||
| ax.legend() | |||
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.
Tests don't like this...
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.
Interesting. Apparently, the legend is placed 'upper right' when no argument is given. Do tests have defaults different from the standard matplotlib defaults?
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.
I thought the default was 'best' instead of 'upper right'?
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.
maybestyle="mpl20"? Otherwise I think it uses classic... But its for a test, so maybe being explicit is better than relying on the style choice?
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, so the tests are still classic.
It's a bit unintuitive to have classic tests, because the code will behave different from more recent (v2) expectations. I see the point of not wanting to update all the tests just for the style though. Probably we should convert tests to mpl20 whenever we have to update the image anyway. That way, we'll get tests with mpl20 behavior in the long run without any additional overhead.
For now, I've put the explicitloc='best' back in.
On 2018/05/09 7:27 AM, Tim Hoffmann wrote: Probably we should convert tests to mpl20 whenever we have to update the image anyway. That way, we'll get tests with mpl20 behavior in the long run without any additional overhead. Yes, that is the intended strategy. |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
For better readability use
legend(loc='upper right')instead oflegend(loc=1)The numbers will remain valid but won't be used anywhere in the docs and only very rarely in internal code.