Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Don't let margins expand polar plots to negative radii by default.#13980
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
| @image_comparison(baseline_images=['streamplot_masks_and_nans'], | ||
| tol=0.04 if on_win else 0, | ||
| remove_text=True, style='mpl20') | ||
| tol=.1, remove_text=True, style='mpl20') |
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 bump the tolerance instead of replacing the test image?
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.
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 definitely shouldn't bump tolerances because the actual test image has changed, as this means any small changes to the images in future will not be picked up.
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.
Sure, I just forced the axes limits to their old values instead...
lib/matplotlib/axes/_base.py Outdated
| # tolerances (whose value come from isclose()) must be used due to | ||
| # floating point issues with streamplot. | ||
| # Index of largest element < x0 + tol, if any. | ||
| i0 = stickies.searchsorted(x0 + 1e-5 * abs(x0) + 1e-8) - 1 |
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.
What is the1e-8 doing for us here that the1e-5*abs(x0) is not?
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.
x0 could be equal to zero and we would still need some slop... the actual value comes from what isclose() uses, I didn't really want to bother finding something better (really the correct thing to do should be to fix streamplot() so that we don't need slop at all, but that's a separate project).
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 factor that out into a function? This is used twice and the function removes the magic numbers.
def tol(x, rtol=1e-5, atol=1e-8): """A tolerance for x such that x+/-tol is considered close to x.""" return rtol * abs(x) + atolThere 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.
That doesn't really work because in one case it's plus and in the other it's minus. I moved the values to their own variables, though.
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.
But that would just bex0 + tol(x0) andx1 - tol(x1) in the code.
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.
sure, pushed
lib/matplotlib/axes/_base.py Outdated
| if self.get_xscale().lower() == 'log': | ||
| x_stickies = x_stickies[x_stickies > 0] | ||
| if self.get_yscale().lower() == 'log': | ||
| y_stickies = y_stickies[y_stickies > 0] | ||
| else: # Small optimization. | ||
| x_stickies,y_stickies =[], [] | ||
| x_stickies =y_stickies =np.array([]) |
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.
Is it a problem that these are now the same object? I assume we do not mutate these values below?
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.
Technically, this is probably working. However, I'd prefer
x_stickies = np.array([])y_stickies = np.array([])First, we don't know if these might be written to for some reason in the future. Second, it keeps the symmetry.
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 moved the conditional higher to avoid this need.
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.
Left one comment about the numerics of 'close'.
cce31fa to0a035aaCompareI also reverted the sole image change by instead adding a check that the "correct limits" are applied, and then forcing the limits to the old values. |
This is implemented by altering the semantics of sticky edges asdocumented in the changelog. As it turns out, this change also improvesconsistency for streamplot():- in test_streamplot.py::test_{linewidth,mask_and_nans}, the change is necessary because the axes limits would now be (-3, 3) (matching the vector field limits), whereas they were previously xlim=(-3.0, 2.9999999999999947), ylim=(-3.0000000000000004, 2.9999999999999947) (they can be inspected with `plt.gcf().canvas.draw(); print(plt.gca().get_xlim(), plt.gca().get_ylim())`).- in test_streamplot.py::test_maxlength, note that the previous version expanded the axes limits *beyond* (-3, 3), whereas the current doesn't do so anymore. It doesn't actually make much sense if the vector field limits are applied if the streamplot goes all the way to the edges, but are ignored otherwise.
This is implemented by altering the semantics of sticky edges as
documented in the changelog. As it turns out, this change also improves
consistency for streamplot():
in test_streamplot.py::test_{linewidth,mask_and_nans}, the small bump
in tolerance is necessary because the axes limits are now (-3, 3)
(matching the vector field limits), whereas they were previously
xlim=(-3.0, 2.9999999999999947), ylim=(-3.0000000000000004, 2.9999999999999947)
(they can be inspected with
plt.gcf().canvas.draw(); print(plt.gca().get_xlim(), plt.gca().get_ylim())).in test_streamplot.py::test_maxlength, note that the previous version
expanded the axes limitsbeyond (-3, 3), whereas the current doesn't
do so anymore. It doesn't actually make much sense if the vector
field limits are applied if the streamplot goes all the way to the
edges, but are ignored otherwise.
Closes#13292.
PR Summary
PR Checklist