Update polar log transform test values to correct matplotlib 3.7 representation #2366

Carifio24 · 2023-02-21T18:43:14Z

One of the CI tests that has been failing lately is the test of ProjectionMplTransform in the case of a polar projection with a radial log axis. I believe the reason for the error is the fix in matplotlib PR 24825, which was added in 3.7.0. This PR aims to fix this test.

Plotting out the test points by hand, the new values in this PR are exactly what one would expect to see - at least to me, the old values don't make sense (hence the need for an upstream fix). Since this is a bug in old versions of matplotlib, I decided to mark the test as an xfail on an older MPL version. Along the way, I created a make_xfailer similar to make_skipper, and did a bit of refactoring since they have the same basic idea.

dhomeier · 2023-02-28T00:28:15Z

Thanks for the updates; I did not have a good idea how the polar log example was supposed to look like, but from the discussion around matplotlib/matplotlib#24825 get that it really was broken before 3.7.

Since this is a bug in old versions of matplotlib, I decided to mark the test as an xfail on an older MPL version. Along the way, I created a make_xfailer similar to make_skipper, and did a bit of refactoring since they have the same basic idea.

Wondering if this could be further generalised to also trigger on _ge_NN conditions (e.g. with a kwarg to make_marker etc.) – asking because for the moment I'd prefer to mark the remaining 4 tests xfail until #2368 is resolved, so the publish CI can at least run.

Carifio24 · 2023-02-28T14:29:59Z

Do you mean a kwarg that reverses the direction of the version check? If so then I can add that in.

dhomeier · 2023-02-28T15:27:24Z

Yes, so one could also define a xfail_matplotlib_ge_37 etc.
Maybe patching MplPolygonalROI will do for a quick fix of the legend tests now, but this could still be useful.

Carifio24 · 2023-02-28T16:31:42Z

@dhomeier I've just pushed a commit to allow make_marker to be more flexible - one can choose which operation (lt, gt, le, ge) to check for.

So for example, to make xfail_matplotlib_ge_37, one would do

MATPLOTLIB_LT_37, xfail_matplotlib_ge_37 = make_xfailer('matplotlib', version='3.7', xfail_if='ge')

dhomeier

Awesome, thanks! Just a few style suggestions, should be ready to go then when CI passes (except for the 4 known ones).

dhomeier · 2023-02-28T16:56:46Z

glue/tests/helpers.py

+                raise ValueError("mark_if must be one of %s" % options)
+            assert not getattr(Version(version_installed), '__%s__' % mark_if)(Version(version))


Suggested change

raise ValueError("mark_if must be one of %s" % options)

assert not getattr(Version(version_installed), '__%s__' % mark_if)(Version(version))

raise ValueError(f"`mark_if` must be one of {options}")

assert not getattr(Version(version_installed), f'__{mark_if}__')(Version(version))

could do some code cleanup on that occasion

dhomeier · 2023-02-28T16:57:46Z

glue/tests/helpers.py

+        return pytest.mark.skipif(str(not installed), reason='Requires %s' % lbl)
+    return make_marker(mark_creator, module, label=label, version=version, mark_if=skip_if)
+
+
+def make_xfailer(module, label=None, version=None, xfail_if='lt'):
+    def mark_creator(installed, lbl, vrs):
+        return pytest.mark.xfail(condition=not installed, reason='Fails if %s < %s' % (lbl, vrs))


Suggested change

return pytest.mark.skipif(str(not installed), reason='Requires %s' % lbl)

return make_marker(mark_creator, module, label=label, version=version, mark_if=skip_if)

def make_xfailer(module, label=None, version=None, xfail_if='lt'):

def mark_creator(installed, lbl, vrs):

return pytest.mark.xfail(condition=not installed, reason='Fails if %s < %s' % (lbl, vrs))

return pytest.mark.skipif(str(not installed), reason=f'Requires {lbl} not {skip_if} {_vrs}')

return make_marker(mark_creator, module, label=label, version=version, mark_if=skip_if)

def make_xfailer(module, label=None, version=None, xfail_if='lt'):

def mark_creator(installed, lbl, vrs):

return pytest.mark.xfail(condition=not installed, reason=f'Fails if {lbl} {xfail_if} {vrs}')

Using "if A [not] le B" in reason is probably easiest, although it may sound a bit clumsy in make_skipper

dhomeier · 2023-02-28T16:58:55Z

glue/core/tests/test_roi_transforms.py

@@ -3,6 +3,7 @@

 from glue.core.roi_pretransforms import ProjectionMplTransform
 from glue.core.state import GlueSerializer, GlueUnSerializer
+from glue.tests.helpers import xfail_matplotlib_le_37


Suggested change

from glue.tests.helpers import xfail_matplotlib_le_37

from glue.tests.helpers import xfail_matplotlib_lt_37

Should be clearer as it fails on < 3.7.0

dhomeier · 2023-02-28T16:59:10Z

glue/core/tests/test_roi_transforms.py

@@ -28,13 +29,14 @@ def test_simple_polar_mpl_transform():
    assert_allclose(new_y, y, rtol=1e-14)


+@xfail_matplotlib_le_37


Suggested change

@xfail_matplotlib_le_37

@xfail_matplotlib_lt_37

dhomeier · 2023-02-28T16:59:51Z

glue/tests/helpers.py



 ASTROPY_INSTALLED, requires_astropy = make_skipper('astropy', label='Astropy')

 MATPLOTLIB_GE_22, requires_matplotlib_ge_22 = make_skipper('matplotlib', version='2.2')

+MATPLOTLIB_GE_37, xfail_matplotlib_le_37 = make_xfailer('matplotlib', version='3.7')


Suggested change

MATPLOTLIB_GE_37, xfail_matplotlib_le_37 = make_xfailer('matplotlib', version='3.7')

MATPLOTLIB_GE_37, xfail_matplotlib_lt_37 = make_xfailer('matplotlib', version='3.7')

See above; just the naming, the condition should be correct.

Carifio24 · 2023-02-28T20:13:10Z

Requested changes have been made

dhomeier · 2023-02-28T20:51:13Z

Great, thanks again!

Carifio24 added 3 commits February 20, 2023 11:52

Update expected polar log transform values to match MPL fix.

5830949

Fix typo in ROI test filename.

de1280c

xfail polar log transform test if matplotlib < 3.7.

b489bd9

This was referenced Feb 23, 2023

TST: Fix typo in test case #2367

Merged

Drawn ROIs affect legend placement in matplotlib 3.7 #2368

Closed

Allow make_marker to be more flexible.

bb74a4c

Carifio24 force-pushed the polar-log-transform-test branch from 5ebed46 to bb74a4c Compare February 28, 2023 16:29

dhomeier requested changes Feb 28, 2023

View reviewed changes

Adjust test mark reason text and variable naming.

01b5934

dhomeier approved these changes Feb 28, 2023

View reviewed changes

dhomeier added bug matplotlib labels Feb 28, 2023

dhomeier changed the title ~~Update polar log transform test values~~ Update polar log transform test values to correct matplotlib 3.7 representation Feb 28, 2023

dhomeier merged commit 11a5dde into glue-viz:main Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update polar log transform test values to correct matplotlib 3.7 representation #2366

Update polar log transform test values to correct matplotlib 3.7 representation #2366

Carifio24 commented Feb 21, 2023

dhomeier commented Feb 28, 2023

Carifio24 commented Feb 28, 2023

dhomeier commented Feb 28, 2023

Carifio24 commented Feb 28, 2023

dhomeier left a comment

dhomeier Feb 28, 2023

dhomeier Feb 28, 2023 •

edited

Loading

dhomeier Feb 28, 2023

dhomeier Feb 28, 2023

dhomeier Feb 28, 2023

Carifio24 commented Feb 28, 2023

dhomeier commented Feb 28, 2023

		raise ValueError("mark_if must be one of %s" % options)
		assert not getattr(Version(version_installed), '__%s__' % mark_if)(Version(version))

	from glue.tests.helpers import xfail_matplotlib_le_37
	from glue.tests.helpers import xfail_matplotlib_lt_37

		@@ -28,13 +29,14 @@ def test_simple_polar_mpl_transform():
		assert_allclose(new_y, y, rtol=1e-14)


		@xfail_matplotlib_le_37

	MATPLOTLIB_GE_37, xfail_matplotlib_le_37 = make_xfailer('matplotlib', version='3.7')
	MATPLOTLIB_GE_37, xfail_matplotlib_lt_37 = make_xfailer('matplotlib', version='3.7')

Update polar log transform test values to correct matplotlib 3.7 representation #2366

Update polar log transform test values to correct matplotlib 3.7 representation #2366

Conversation

Carifio24 commented Feb 21, 2023

dhomeier commented Feb 28, 2023

Carifio24 commented Feb 28, 2023

dhomeier commented Feb 28, 2023

Carifio24 commented Feb 28, 2023

dhomeier left a comment

Choose a reason for hiding this comment

dhomeier Feb 28, 2023

Choose a reason for hiding this comment

dhomeier Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

dhomeier Feb 28, 2023

Choose a reason for hiding this comment

dhomeier Feb 28, 2023

Choose a reason for hiding this comment

dhomeier Feb 28, 2023

Choose a reason for hiding this comment

Carifio24 commented Feb 28, 2023

dhomeier commented Feb 28, 2023

dhomeier Feb 28, 2023 •

edited

Loading