Axisartist testing + bugfixes #7545

QuLogic · 2016-12-01T22:10:16Z

The axisartist toolkit does not appear to have any testing done at all; it was still importing matplotlib.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 in 2316a07, a7dde13, and 8062fe5; 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.

QuLogic

For future reference.

QuLogic · 2016-12-04T09:21:25Z

lib/mpl_toolkits/tests/test_axisartist_axis_artist.py

+
+@image_comparison(baseline_images=['axis_artist_ticks'],
+                  extensions=['png'], style='default')
+def test_ticks():


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?

This is fixed by d8ada7a.

lib/mpl_toolkits/tests/test_axisartist_angle_helper.py

tacaswell · 2016-12-04T22:52:04Z

For reference, the issue with missed import patch up does not exist on 2.x.

#5752 re-arranged the files on master, #6556 reverted the vendoring on the 2.x branch which was then merged up to master, but missed the new files.

QuLogic · 2018-02-06T04:49:23Z

This is rebased and working. Seems I didn't have to update any images in a while too, so that's good.

afvincent

Apart from one real question in set_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.

afvincent · 2018-02-06T05:13:29Z

lib/mpl_toolkits/axisartist/angle_helper.py

-    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"))


Is there a reason not to simply use

self.den = int(nbins)

?

That's a bug (evidently not tested enough.)

lib/mpl_toolkits/axisartist/angle_helper.py

lib/mpl_toolkits/tests/test_axisartist_grid_helper_curvelinear.py

afvincent · 2018-02-06T06:09:29Z

lib/mpl_toolkits/tests/test_axisartist_grid_helper_curvelinear.py

+    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


Just to be sure: switching from 2, 10 to 2, 12 was done on purpose?

It just looked better, IIRC.

Fair enough :)

afvincent · 2018-02-06T06:09:56Z

lib/mpl_toolkits/tests/test_axisartist_grid_helper_curvelinear.py

+                                                     lon_minmax=None,
+                                                     lat_minmax=(0, np.inf))
+
+    grid_locator1 = angle_helper.LocatorDMS(12)


Just to be sure: switching from 5 to 12 was done on purpose?

This is probably from curvelinear_test3; there's also curvelinear_test2 which I think I squashed into one test because they're pretty much the same.

afvincent · 2018-02-06T06:14:24Z

lib/mpl_toolkits/tests/test_axisartist_grid_helper_curvelinear.py

+
+    ax1.set_aspect(1.)
+    ax1.set_xlim(-8, 8)
+    ax1.set_ylim(-4, 12)


Switching from (-5, 12) to (-8, 8), and from (-5, 10) to (-4, 12) was done on purpose?

Same here; looked better.

lib/mpl_toolkits/tests/test_axisartist_axis_artist.py

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 there are no expected results, so it's up to whoever runs it to check. I'm not even completely sure the results are right, but they seem acceptable.

The results seem to be acceptable.

It raises "TypeError: Cannot cast ufunc subtract output from dtype('float64') to dtype('int64') with casting rule 'same_kind'" due to in-place subtraction.

Instead of clipping the entire path+marker to the gc's clip path (which is 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 axis appears to be broken.

The results seem to be acceptable.

efiring

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.

efiring · 2018-02-09T00:07:36Z

@afvincent do you have any reservations about this?

afvincent · 2018-02-09T00:19:40Z

@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?

efiring · 2018-02-09T02:05:16Z

Those formatters are undocumented and nearly incomprehensible.
As far as I can see, the fmt_ds format string is never used; it would be used only if factor is 1 and number_fraction is not None, but _get_number_fraction(self, 1) returns None as the number fraction. So I don't think this is an API change.

QuLogic · 2018-02-09T02:16:52Z

I think @afvincent is referring to this small change:

it didn't make any sense to me that the decimal place would go before the symbol and the fractional part after.

(unless this never happens; I don't recall now whether I checked before.)

afvincent · 2018-02-09T02:30:16Z

Yes I was referring to changes done in 51e5434, 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)” ;).

afvincent

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...

efiring · 2018-02-09T03:44:26Z

I was referring to one example of the 51e5434 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.

anntzer · 2020-04-23T21:28:27Z

lib/mpl_toolkits/axisartist/axis_artist.py

+            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])):


@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) to loc 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.

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 fixed something, 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.py index 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.

OK, let's not bother for now then.

QuLogic added topic: mpl_toolkit topic: testing labels Dec 1, 2016

QuLogic added this to the 2.1 (next point release) milestone Dec 1, 2016

QuLogic mentioned this pull request Dec 4, 2016

Cleanup: sorted, dict iteration, array.{ndim,size}, ... #7549

Merged

QuLogic commented Dec 4, 2016

View reviewed changes

QuLogic force-pushed the axisartist-testing branch from 8062fe5 to 996976a Compare January 2, 2017 08:32

QuLogic mentioned this pull request Jan 10, 2017

anchored_artists don't support zorder argument #7650

Closed

2 tasks

tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017

QuLogic mentioned this pull request Sep 26, 2017

Cleanup some toolkit six imports #9237

Merged

QuLogic force-pushed the axisartist-testing branch from 996976a to bd36aa3 Compare February 6, 2018 04:05

QuLogic changed the title ~~[WIP] Axisartist testing + bugfixes~~ Axisartist testing + bugfixes Feb 6, 2018

afvincent reviewed Feb 6, 2018

View reviewed changes

QuLogic added 13 commits February 6, 2018 02:56

Use @Property decorator in axisartist.

5458d4e

Simplify FormatterDMS/FormatterHMS tick strings.

51e5434

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.

Fix bug in signs of FormatterDMS/HMS.

e50c059

Make axisartist.angle_helper pseudo-tests real.

7a64b79

These do no good sitting here and not running, especially because there are no expected results, so it's up to whoever runs it to check. I'm not even completely sure the results are right, but they seem acceptable.

Add sign when FormatterDMS emits only seconds.

15fb901

Also test FormatterHMS and select_step24.

f467768

Make axisartist.clip_path pseudo-tests real.

bc1bfab

The results seem to be acceptable.

Fix axisartist.floating_axes error with new NumPy.

bcd41e4

It raises "TypeError: Cannot cast ufunc subtract output from dtype('float64') to dtype('int64') with casting rule 'same_kind'" due to in-place subtraction.

Don't double-strike axis artist Ticks.

ffb5461

Fix clipping of axis artist Ticks.

7ecf80f

Instead of clipping the entire path+marker to the gc's clip path (which is 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.

Make axisartist.axislines pseudo-tests real.

bf25cf6

The results seem to be acceptable.

Make axisartist.grid_helper_curvelinear pseudo-tests real.

e0556d1

The results seem to be acceptable, though the tick direction of the axis appears to be broken.

Make axisartist.floating_axes pseudo-tests real.

ff53c3f

The results seem to be acceptable.

QuLogic added 3 commits February 6, 2018 03:02

Make axisartist.axis_artist pseudo-tests real.

07ed9c5

The results seem to be acceptable.

Make axisartist.grid_finder pseudo-tests real.

9488d1c

Fix minor typos.

b614316

QuLogic force-pushed the axisartist-testing branch from bd36aa3 to b614316 Compare February 6, 2018 08:04

tacaswell mentioned this pull request Feb 8, 2018

import matplotlib.pyplot as plt in axis_artist.py #10401

Closed

efiring approved these changes Feb 9, 2018

View reviewed changes

afvincent approved these changes Feb 9, 2018

View reviewed changes

efiring merged commit 4174179 into matplotlib:master Feb 9, 2018

QuLogic deleted the axisartist-testing branch February 9, 2018 03:47

QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018

anntzer reviewed Apr 23, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Axisartist testing + bugfixes #7545

Axisartist testing + bugfixes #7545

QuLogic commented Dec 1, 2016 •

edited

Loading

QuLogic left a comment

QuLogic Dec 4, 2016

QuLogic Feb 6, 2018

tacaswell commented Dec 4, 2016

QuLogic commented Feb 6, 2018

afvincent left a comment

afvincent Feb 6, 2018

QuLogic Feb 6, 2018

afvincent Feb 6, 2018

QuLogic Feb 6, 2018

afvincent Feb 8, 2018

afvincent Feb 6, 2018

QuLogic Feb 6, 2018

afvincent Feb 6, 2018

QuLogic Feb 6, 2018

efiring left a comment

efiring commented Feb 9, 2018

afvincent commented Feb 9, 2018

efiring commented Feb 9, 2018

QuLogic commented Feb 9, 2018

afvincent commented Feb 9, 2018

afvincent left a comment

efiring commented Feb 9, 2018

anntzer Apr 23, 2020

QuLogic Apr 24, 2020

anntzer Apr 24, 2020

Axisartist testing + bugfixes #7545

Axisartist testing + bugfixes #7545

Conversation

QuLogic commented Dec 1, 2016 • edited Loading

QuLogic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Dec 4, 2016

QuLogic commented Feb 6, 2018

afvincent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiring left a comment

Choose a reason for hiding this comment

efiring commented Feb 9, 2018

afvincent commented Feb 9, 2018

efiring commented Feb 9, 2018

QuLogic commented Feb 9, 2018

afvincent commented Feb 9, 2018

afvincent left a comment

Choose a reason for hiding this comment

efiring commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuLogic commented Dec 1, 2016 •

edited

Loading