[MRG] MAINT: add fixture for init and clean-up with matplotlib #13708

glemaitre · 2019-04-24T13:31:20Z

Instead of using our own decorator to skip tests if matplotlib is not installed, we should use pytest.

thomasjpfan · 2019-04-24T21:31:06Z

There are a bunch of “close_figures” that a dev needs to remember to call. It may be good to have a fixture that yields a figure and cleans it up.

glemaitre · 2019-04-24T22:06:59Z

There are a bunch of “close_figures” that a dev needs to remember to call. It may be good to have a fixture that yields a figure and cleans it up.

I think that sometimes the figure will be created within the function itself (at least for some part of the test). We could have a teardown at the level of the module which could clean-up all figures. We could also skip this teardown if matplotlib is not available. WDYT?

thomasjpfan · 2019-04-25T13:21:10Z

We could have a teardown at the level of the module which could clean-up all figures.

Would there be any memory issues if a module generates a bunch of figures?

glemaitre · 2019-04-25T14:21:20Z

Would there be any memory issues if a module generates a bunch of figures?

I don't think that it would be a problem. If this is the case we could move from module to function.

thomasjpfan · 2019-04-25T14:41:50Z

A "hacky" way of using teardown_function(function) would be to check to name:

def teardown_function(function):
    if 'plot' in function.__name__:
        close_figure()

WDYT?

glemaitre · 2019-04-25T15:15:21Z

It would require to call all the function with plot within the name which could also be error-prone.

thomasjpfan · 2019-04-25T15:29:40Z

within the name which could also be error-prone.

I see your point. The following is slightly better:

def teardown_function(function):
    if '_plot_' in function.__name__:
        close_figure()

What about it fixture?

@pytest.fixture
def close_figures():
    yield
    if has_matplotlib():
    	close_figure()

def test_stuff(close_figures):
    ....

The fixture is not much better than having uses call close_figure themselves.

glemaitre · 2019-04-25T15:37:10Z

Regarding the fixture, this is the reason that I proposed directly the teardown because you don't have to pass anything to the function.

thomasjpfan

Okay lets go with module level teardowns.

NicolasHug · 2019-04-25T19:59:45Z

sklearn/utils/testing.py

-        else:
-            return func(*args, **kwargs)
-    return run_test
+def has_matplotlib():


we have a check_matplotlib_support util already

check_matplotlib_support raise an error while I only need a boolean test.
Do you mean to call check_matplotlib_support within this helper?

Ok yeah they're not doing the same thing.

Can we make it private to this file then? It's only really useful for the decorator

NicolasHug · 2019-04-25T20:02:35Z

sklearn/inspection/tests/test_partial_dependence.py

 def test_plot_partial_dependence():
    # Test partial dependence plot function.
-    import matplotlib.pyplot as plt  # noqa
+    plt = pytest.importorskip('matplotlib.pyplot')


IMO having a decorator makes it more obvious that the test is matplotlib related.

Also, it is strange to use this pattern in some places while the skip_if_no_matplotlib decorator is used in some others

I'm split. it was the suggestion of @ogrisel IRL. His point was to say that we have a decorator and then a lazily import while now you have only the import which skip the test if required.

I'll like the mark.skipif since it can be used in different setups.

why not just avoid the decorator and always use pytest.importorskip('matplotlib.pyplot') then?

I'd rather keep things consistent.

But I'm OK too with decorator + lazy import.

I find the idiom:

plt = pytest.importorskip('matplotlib.pyplot')

good enough. I don't think we need a decorator for that.

sklearn/ensemble/tests/test_partial_dependence.py

NicolasHug · 2019-04-25T20:08:18Z

sklearn/inspection/tests/test_partial_dependence.py

@@ -45,6 +46,11 @@
 multioutput_regression_data = (make_regression(n_targets=2, random_state=0), 2)


+def teardown_module(module):
+    if has_matplotlib():
+        close_figure()


So does that mean we don't need to call close_figure() in all the tests of this module anymore?

If that's the case, I would suggest changing the defualt of close_figure() from None to 'all' for it to be more obvious.

So does that mean we don't need to call close_figure() in all the tests of this module anymore?

This is the idea.

If that's the case, I would suggest changing the defualt of close_figure() from None to 'all' for it to be more obvious.

We can do that.

if we do, we need this to be merged before the release

NicolasHug · 2019-04-25T20:54:23Z

Also can you please explain why we need to clean the figures @glemaitre ?

I would have expected the figures to be GCed at the end of each function, or at the very least at the end of the module (hence making module_teardown() useless)

glemaitre · 2019-04-26T10:11:32Z

I am afraid it does not.

I tried the following test

@pytest.mark.parametrize(
    "x", list(range(30))
)
def test_fig(x):
    import matplotlib.pyplot as plt
    fig, ax = plt.subplots()

and got the following warning:

/home/glemaitre/miniconda3/envs/dev/lib/python3.7/site-packages/matplotlib/pyplot.py:514: RuntimeWarning: More than 20 figures have been opened. Figures created through the pyplot interface (`matplotlib.pyplot.figure`) are retained until explicitly closed and may consume too much memory. (To control this warning, see the rcParam `figure.max_open_warning`).
  max_open_warning, RuntimeWarning)

thomasjpfan · 2019-04-26T11:49:57Z

Does that mean if a module creates more than 20 figures the tests will fail? If so, we should close revert back to calling close_figure at the end of each test.

glemaitre · 2019-04-26T11:54:41Z

Does that mean if a module creates more than 20 figures the tests will fail? If so, we should close revert back to calling close_figure at the end of each test.

Nop this is just a warning, so everything will be fine, we will just have an increase of memory until we close all figure in the teardown.

thomasjpfan · 2019-04-26T12:08:31Z

I suspect we will be adding more plotting functionality soon, and having increased memory usage would be unfavorable.

I am in favor of cleaning up memory after each test. (Function level)

NicolasHug · 2019-04-26T12:08:58Z

A few other points:

If we use the teardown, why do we still keep the close_figure() calls?
We should just remove the helper close_figure() because it's just a (bad) wrapper around plt.close: https://matplotlib.org/api/_as_gen/matplotlib.pyplot.close.html
I'm actually going to open a PR for this so that it can be merged before 0.21.
I still don't understand why / whether the figures are closed at the end of a module / function

NicolasHug · 2019-04-26T12:16:45Z

Opened #13730, can you guys look at it please

NicolasHug · 2019-04-26T12:22:06Z

@glemaitre I cannot reproduce your warning

import pytest
import matplotlib.pyplot as plt

@pytest.mark.parametrize(
    "x", list(range(300))
)
def test_fig(x):
    fig, ax = plt.subplots()

print(plt.get_fignums())

no warnings
list is empty

thomasjpfan · 2019-04-26T13:44:38Z

@NicolasHug I am able to reproduce the warning with your script. It could be platform/matplotlib version specific?

thomasjpfan · 2019-04-26T14:20:23Z

Going through how yellowbrick and pandas test figures, closing the figure after each plotting test case is common amongst them. We need to make sure that plotting tests manually closes figures.

NicolasHug · 2019-04-26T14:33:01Z

OK. I agree we should be doing whatever pandas or yellowbrick is doing. Let's get rid of the module teardown.

glemaitre · 2019-04-29T10:01:05Z

OK I make the modifications

glemaitre · 2019-05-03T14:07:10Z

We install in dev mode in linux so it finds the conftest.

I'll then add a conftest in sklearn and add an exception in the corresponding test.

glemaitre · 2019-05-03T15:05:00Z

I added one of the windows build and it seems that in this case the combining is working.

sklearn/conftest.py

build_tools/azure/install.sh

doc/developers/contributing.rst

sklearn/conftest.py

sklearn/utils/testing.py

NicolasHug · 2019-05-04T19:18:30Z

sklearn/utils/testing.py

@@ -1024,21 +1015,3 @@ def assert_run_python_script(source_code, timeout=60):
                               % e.output.decode('utf-8'))
    finally:
        os.unlink(source_file)
-
-
-def close_figure(fig=None):


I'm happy to see this go, but this is public and it's going to be in 0.21...

:s

Maybe we could squeeze it in 0.21 ping @jnothman

If it is only introduced in 0.21, let's make it private before final release.

I can't code right now, but will be happy to approve a PR which does

Why no just merging this PR and remove this function directly?

If this is merged before release then fine. But in case it is not, a quick fix would do

I think that we converged toward a concensus on what to implement. I can quickly answer to review so if anyone have some additional changes to bring, I'll be happy to make them.

yes in another pr

sklearn/conftest.py

doc/developers/contributing.rst

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

NicolasHug

LGTM apart from this

build_tools/azure/install.sh

doc/developers/contributing.rst

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

…rn into is/matplotlib_maint

doc/developers/contributing.rst

thomasjpfan

With this PR, we will have matplotlib testing for 2 linux instances and 1 windows instances. (It looks like windows was added since we can report coverage correctly)

@ogrisel Your idea of using pyplot as a fixture is great!

glemaitre · 2019-05-05T20:01:02Z

With this PR, we will have matplotlib testing for 2 linux instances and 1 windows instances. (It looks like windows was added since we can report coverage correctly)

Yes. 2 linux for the minimum and latest and windows for the reporting. I think that we should investigate why the combining in linux does not work as expected.

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

thomasjpfan · 2019-05-05T21:34:00Z

I think that we should investigate why the combining in linux does not work as expected.

Want to figure this out in another PR? I think it is important we remove close_figure from the release.

glemaitre · 2019-05-06T07:11:59Z

Yes in another PR is fine.

ogrisel · 2019-05-06T07:42:48Z

@jnothman I let you backport that to be included in the 0.21 release branch.

…t-learn#13708)

glemaitre added 3 commits April 24, 2019 15:28

MAINT: use pytest decorator instead of our decorator for matplotlib

9fdbe3e

use importskip wihthin the function

a982802

iter

ca4afe3

use teardown function

5dda183

hotfix

fd70c12

thomasjpfan approved these changes Apr 25, 2019

View reviewed changes

NicolasHug reviewed Apr 25, 2019

View reviewed changes

glemaitre added 2 commits April 29, 2019 11:42

Merge remote-tracking branch 'origin/master' into is/matplotlib_maint

2b97e8d

use matplotlib style

f033e18

add matplotlib to one of the windows built

e3d033a

glemaitre changed the title ~~MAINT: use pytest skipif instead of our decorator for matplotlib~~ [MRG] MAINT: use pytest skipif instead of our decorator for matplotlib May 3, 2019

glemaitre changed the title ~~[MRG] MAINT: use pytest skipif instead of our decorator for matplotlib~~ [MRG] MAINT: add fixture for init and clean-up with matplotlib May 3, 2019

jnothman reviewed May 4, 2019

View reviewed changes

sklearn/conftest.py Outdated Show resolved Hide resolved

rename plt to pyplot

3db0d6a

NicolasHug reviewed May 4, 2019

View reviewed changes

NicolasHug and others added 6 commits May 4, 2019 21:35

Update doc/developers/contributing.rst

a4bf25c

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

Apply suggestions from code review

31fb7f0

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

remaining comments

028e2ed

iter

e47f883

iter

ca7de61

iter

6dd97c8

NicolasHug approved these changes May 5, 2019

View reviewed changes

build_tools/azure/install.sh Outdated Show resolved Hide resolved

doc/developers/contributing.rst Outdated Show resolved Hide resolved

doc/developers/contributing.rst Outdated Show resolved Hide resolved

doc/developers/contributing.rst Outdated Show resolved Hide resolved

NicolasHug and others added 3 commits May 5, 2019 17:41

Update doc/developers/contributing.rst

64dc513

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

addressed nicolas comments

448765f

Merge branch 'is/matplotlib_maint' of github.com:glemaitre/scikit-lea…

3f5033c

…rn into is/matplotlib_maint

NicolasHug reviewed May 5, 2019

View reviewed changes

doc/developers/contributing.rst Outdated Show resolved Hide resolved

thomasjpfan approved these changes May 5, 2019

View reviewed changes

Update doc/developers/contributing.rst

7ce4d33

Co-Authored-By: glemaitre <g.lemaitre58@gmail.com>

ogrisel approved these changes May 6, 2019

View reviewed changes

ogrisel merged commit b34751b into scikit-learn:master May 6, 2019

ogrisel added this to the 0.21 milestone May 6, 2019

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request May 6, 2019

[MRG] MAINT: add fixture for init and clean-up with matplotlib (sciki…

3665df3

…t-learn#13708)

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019

[MRG] MAINT: add fixture for init and clean-up with matplotlib (sciki…

54a7ea1

…t-learn#13708)

Uh oh!

[MRG] MAINT: add fixture for init and clean-up with matplotlib #13708

[MRG] MAINT: add fixture for init and clean-up with matplotlib #13708

Uh oh!

Conversation

glemaitre commented Apr 24, 2019

Uh oh!

thomasjpfan commented Apr 24, 2019

Uh oh!

glemaitre commented Apr 24, 2019

Uh oh!

thomasjpfan commented Apr 25, 2019

Uh oh!

glemaitre commented Apr 25, 2019

Uh oh!

thomasjpfan commented Apr 25, 2019

Uh oh!

glemaitre commented Apr 25, 2019

Uh oh!

thomasjpfan commented Apr 25, 2019

Uh oh!

glemaitre commented Apr 25, 2019

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug commented Apr 25, 2019

Uh oh!

glemaitre commented Apr 26, 2019

Uh oh!

thomasjpfan commented Apr 26, 2019

Uh oh!

glemaitre commented Apr 26, 2019

Uh oh!

thomasjpfan commented Apr 26, 2019

Uh oh!

NicolasHug commented Apr 26, 2019

Uh oh!

NicolasHug commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasHug commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasjpfan commented Apr 26, 2019

Uh oh!

thomasjpfan commented Apr 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasHug commented Apr 26, 2019

Uh oh!

glemaitre commented Apr 29, 2019

Uh oh!

glemaitre commented May 3, 2019

Uh oh!

glemaitre commented May 3, 2019

Uh oh!

Uh oh!

Uh oh!

NicolasHug commented Apr 26, 2019 •

edited

Loading

NicolasHug commented Apr 26, 2019 •

edited

Loading

thomasjpfan commented Apr 26, 2019 •

edited

Loading