Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Axisartist testing + bugfixes#7545
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
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.
For future reference.
| @image_comparison(baseline_images=['axis_artist_ticks'], | ||
| extensions=['png'], style='default') | ||
| def test_ticks(): |
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.
This test fails because ticks provided by axis-artist are clipped by the gc to the Axes; should I just disable that clipping? What about when ticks are set outside the view limits? Should it do clipping based on that instead?
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.
This is fixed byd8ada7a.
Uh oh!
There was an error while loading.Please reload this page.
This is rebased and working. Seems I didn't have to update any images in a while too, so that's good. |
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.
Apart from one real question inset_param, the rest are very minor questions because I am slightly out of depth here anyway.
Disclaimer: I did not check:
test_select_step*;test_formatters;test_ticks;test_clip_path;
but otherwise, the “new” tests seem indeed to be nice copy-pasted and slightly cleant versions of the previous versions.
| def set_params(self, **kwargs): | ||
| if "nbins" in kwargs: | ||
| def set_params(self, nbins=None): | ||
| if nbins is not None: | ||
| self.den = int(kwargs.pop("nbins")) |
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 there a reason not to simply use
self.den=int(nbins)
?
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.
That's a bug (evidently not tested enough.)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/mpl_toolkits/tests/test_axisartist_grid_helper_curvelinear.py OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| ax1.axis["lat"] = axis = grid_helper.new_floating_axis(0, 45, axes=ax1) | ||
| axis.label.set_text("Test") | ||
| axis.label.set_visible(True) | ||
| axis.get_helper()._extremes = 2, 12 |
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 to be sure: switching from2, 10 to2, 12 was done on purpose?
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.
It just looked better, IIRC.
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.
Fair enough :)
| lon_minmax=None, | ||
| lat_minmax=(0, np.inf)) | ||
| grid_locator1 = angle_helper.LocatorDMS(12) |
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 to be sure: switching from5 to12 was done on purpose?
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.
This is probably fromcurvelinear_test3; there's alsocurvelinear_test2 which I think I squashed into one test because they're pretty much the same.
| ax1.set_aspect(1.) | ||
| ax1.set_xlim(-8, 8) | ||
| ax1.set_ylim(-4, 12) |
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.
Switching from(-5, 12) to(-8, 8), and from(-5, 10) to(-4, 12) was done on purpose?
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.
Same here; looked better.
Uh oh!
There was an error while loading.Please reload this page.
It doesn't really make sense to put the fractional part of the value_after_ the symbol. Plus, there's a bug with non-LaTeX usage because\mkern is not recognized with mathtext.
These do no good sitting here and not running, especially because thereare no expected results, so it's up to whoever runs it to check. I'm noteven completely sure the results are right, but they seem acceptable.
The results seem to be acceptable.
It raises "TypeError: Cannot cast ufunc subtract output fromdtype('float64') to dtype('int64') with casting rule 'same_kind'" due toin-place subtraction.Instead of clipping the entire path+marker to the gc's clip path (whichis inside the Axes only), clip the *position* of the path (aka marker).They then stay within the Axes, but don't get clipped if outgoing.
The results seem to be acceptable.
The results seem to be acceptable, though the tick direction of the axisappears to be broken.
The results seem to be acceptable.
The results seem to be acceptable.
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 haven't examined this in detail, but I think it is a mistake to let it languish. It is cleaning up an otherwise neglected part of the codebase.
@afvincent do you have any reservations about this? |
@efiring Not really for the parts that I understand.@QuLogic fixed the small bug that I had noticed, and my other comments were more curiosity than real requests. And I tend to agree with your analysis about this PR. Should this receive a small “API changes” entry, due to the small changes in the DMS and HMS formatters? |
Those formatters are undocumented and nearly incomprehensible. |
I think@afvincent is referring to this small change:
(unless this never happens; I don't recall now whether I checked before.) |
Yes I was referring to changes done in51e5434, sorry for being vague. Anyway, I do not have a strong opinion about the absolute necessity of an API change entry about that. And even less if we requalify this as a “bugfix (a virtually non public class)” ;). |
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 I was able to check (i.e. far from 100%¹...) seems OK and based on@efiring's points, I also think that it would be nice to merge this PR.
¹ : but all these tests seems to be green flagged by CI anyway...
I was referring to one example of the51e5434 change in which the position of the decimal point was moved, so it could conceivably be an API change--but it wasn't, because it is in an inaccessible code path. |
| marker_rotation.clear().rotate_deg(angle+add_angle) | ||
| locs = path_trans.transform_non_affine([loc]) | ||
| if (self.axes and | ||
| not self.axes.viewLim.contains(locs[0][0], locs[0][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.
@QuLogic I realize this is a bit old, but I think this check is not correct? You've applied part of path_trans (only the non-affine part) toloc at this point, so if path_trans (i.e. self.get_transform()) is not linear the containment check may be not at all what we need?
I don't have a real failing case though (as I'm not sure how to get a log-scaled axisartist...) and am not even sure what the correct containment check would be, 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.
Possibly, but without a specific case, I guess I also don't know whether it's really wrong or how to fix it. It certainly fixedsomething, as indicated in the commit message.
I'm not entirely sure if it's the right test, but I applied:
diff --git a/examples/axisartist/demo_parasite_axes.py b/examples/axisartist/demo_parasite_axes.pyindex d3fe972d4..52066a59c 100644--- a/examples/axisartist/demo_parasite_axes.py+++ b/examples/axisartist/demo_parasite_axes.py@@ -24,8 +24,11 @@ import matplotlib.pyplot as plt fig = plt.figure() host = HostAxes(fig, [0.15, 0.1, 0.65, 0.8])+host.set_yscale('log') par1 = ParasiteAxes(host, sharex=host)+par1.set_yscale('log') par2 = ParasiteAxes(host, sharex=host)+par2.set_yscale('log') host.parasites.append(par1) host.parasites.append(par2)@@ -44,8 +47,8 @@ p2, = par1.plot([0, 1, 2], [0, 3, 2], label="Temperature") p3, = par2.plot([0, 1, 2], [50, 30, 15], label="Velocity") host.set_xlim(0, 2)-host.set_ylim(0, 2)-par1.set_ylim(0, 4)+host.set_ylim(0.001, 2)+par1.set_ylim(0.001, 4) par2.set_ylim(1, 65) host.set_xlabel("Distance")
and it does appear that ticks are visible and that this code is hit.
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.
OK, let's not bother for now then.
Uh oh!
There was an error while loading.Please reload this page.
The
axisartisttoolkit does not appear to have any testing done at all; it was still importingmatplotlib.externals.six! There were a lot of tests written into the files themselves, so I just pulled them out and made them into real tests. There are some bugfixes in2316a07,a7dde13, and8062fe5; not sure if they're worth backporting.I also simplified the format of
FormatterDMS/FormatterHMS; it didn't make any sense to me that the decimal place would go before the symbol and the fractional part after.This will likely fail to pass due to some tick clipping bug that I haven't fixed yet; it's also possible that the flaky comparisons will arise here as well. I have an idea of how to fix that, but I thought I'd write the tests first.