Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DEP: Pending deprecation warning for matrix #10142

Merged
merged 2 commits into from May 30, 2018

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Dec 1, 2017

As discussed on the mailing list. Two commits, which can be split apart if need be

  • make clearer in the docs that we do not recommend using matrix: done in DOC: advise against use of matrix. #10973
  • emit PendingDeprecationWarning in matrix.__new__: this PR; UPDATE: includes silencing for the relevant tests.

The idea is only to warn users to start moving away; deprecation itself will depend on scipy.sparse as well as possible other packages.

Follow-up to this might be to move tests that are specific to matrix to libmatrix instead of scattered throughout the code, to help make a move to a separate package or other location easier.

p.s. Do let me know if I'm missing some obvious pieces of the documentation...

@njsmith
Copy link
Member

njsmith commented Dec 1, 2017

@mhvk can you give a sense of the slowdown the PendingDeprecationWarning gives for some small matrix operations? sometimes the warning module is surprisingly slow so I just want to double-check...

Copy link
Member

@shoyer shoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes!

@@ -210,6 +215,9 @@ class matrix(N.ndarray):
"""
__array_priority__ = 10.0
def __new__(subtype, data, dtype=None, copy=True):
warnings.warn('the matrix subclass is not the recommended way to deal '
'with linear algebra. Please adjust your code to use '
'regular ndarray.', PendingDeprecationWarning)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a nice touch to add a URL pointing users to a longer description/discussion of why matrix is no longer recommended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may also confuse users who aren't attempting to do linear algebra, but simply thought that np.matrix is the default ndarray constructor.

Copy link
Contributor

@ilayn ilayn Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also consider adding stacklevel=2 to warn() such that the warning is raised from the calling function or __main__ but not inside np.matrix.

@eric-wieser
Copy link
Member

I think np.array([1, 1, 2]).view(np.matrix) skips __new__

@@ -309,8 +278,7 @@ Linear Algebra Equivalents
- 2x3 matrix literal

* - ``[ a b; c d ]``
- ``vstack([hstack([a,b]), hstack([c,d])])`` or
``bmat('a b; c d').A``
- ``block([[a,b], [c,d][)``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@mhvk
Copy link
Contributor Author

mhvk commented Dec 1, 2017

@njsmith -on my slow laptop, %timeit m = np.matrix([[0.,1.],[2.,3.]]) goes from 17.5 µs to 21 µs, so it is not horrendous.

@eric-wieser - I thought of doing it in __array_finalize__, but worried about making the impact too big: then every operation gets a 5 µs overhead.

@shoyer - Yes, a link would be better, which probably means that the note for the matrix subclass should be expanded to provide that description in more detail. I ran out of steam... (edits of or PRs to my PR most welcome...)

@mhvk
Copy link
Contributor Author

mhvk commented Dec 1, 2017

All tests with matrix now fail, which I guess means one should guard against the warning. One "advantage" is that it should make it easier to identify and move all those tests...

@eric-wieser
Copy link
Member

but worried about making the impact too big: then every operation gets a 5 µs overhead.

You're right, __new__ is exactly the right place, since that going to almost always be the user calling it.

which I guess means one should guard against the warning.

Perhaps only do so in the matrixlib test files, which will force you to move the tests?

@ghost ghost mentioned this pull request Dec 1, 2017
10 tasks
@njsmith
Copy link
Member

njsmith commented Dec 1, 2017

We should check that whatever scipy.sparse is doing to create matrix objects triggers the warning, since we know that that's a major way to end up with matrix objects without manually using np.matrix.

@stefanv
Copy link
Contributor

stefanv commented Dec 1, 2017

SciPy usage:

compressed.py:        return np.matrix(result, copy=False)
compressed.py:            out = np.matrix(out)
coo.py:        return np.matrix(result, copy=False)
dia.py:            ret = np.matrix(res, dtype=res_dtype)
dia.py:            row_sums = np.matrix(row_sums)
dia.py:            ret = np.matrix(row_sums.sum(axis=axis))
base.py:                result = np.asmatrix(result)
base.py:                result = np.asmatrix(result)
base.py:        return np.asmatrix(self.toarray(order=order, out=out))
base.py:            return (self * np.asmatrix(np.ones(
                (n, 1), dtype=res_dtype))).sum(
                dtype=dtype, out=out)
base.py:            ret = np.asmatrix(np.ones(
                (1, m), dtype=res_dtype)) * self
base.py:            ret = self * np.asmatrix(
                np.ones((n, 1), dtype=res_dtype))
compressed.py:            ret = np.asmatrix(ret)
csr.py:            return np.asmatrix(val)
data.py:        return np.asmatrix(ret)
lil.py:                A = np.asmatrix(arg1)
linalg/interface.py:            y = np.asmatrix(y)
linalg/interface.py:            y = np.asmatrix(y)
linalg/interface.py:            Y = np.asmatrix(Y)
linalg/isolve/utils.py:            x = asmatrix(x)

.. note::
It is strongly advised *not* to use the matrix subclass. As described
below, it makes writing functions that deal consistently with matrices
and regular arrays very difficult. Currently, the main reason still to use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence is incomplete.

Perhaps "Currently, they are mainly used for interacting with scipy.sparse."

- ``<:(`` Element-wise multiplication requires calling a function,
``multiply(A,B)``.
- ``<:(`` The use of operator overloading is a bit illogical: ``*``
does not work element-wise but ``/`` does.
- You interact well with ``scipy.sparse``.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"You interact well" is kind of vague. Same above. Can we be more explicit here?

@@ -210,6 +215,9 @@ class matrix(N.ndarray):
"""
__array_priority__ = 10.0
def __new__(subtype, data, dtype=None, copy=True):
warnings.warn('the matrix subclass is not the recommended way to deal '
'with linear algebra. Please adjust your code to use '
'regular ndarray.', PendingDeprecationWarning)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may also confuse users who aren't attempting to do linear algebra, but simply thought that np.matrix is the default ndarray constructor.

@@ -82,6 +82,8 @@ had to use ``dot`` instead of ``*`` to multiply (reduce) two tensors
(scalar product, matrix vector multiplication etc.). Since Python 3.5 you
Copy link
Member

@ahaldane ahaldane Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above in the intro to this section, I would add in some of the description of np.matrix that you deleted above, otherwise the discussion to matrix seems out of the blue.

Maybe something like

 'array' or 'matrix'? Which should I use?
========================================

Historically, NumPy has provided a special matrix type, `np.matrix`, which 
is a subclass of ndarray which makes binary operations linear algebra 
operations. You may see it used in some existing code instead of `np.array`. 
Which one to use?

@stefanv
Copy link
Contributor

stefanv commented Apr 14, 2018

@mhvk What is the status of this discussion? Should the discussion be summarized in a short NEP, or should we simply move forward with a few PRs?

@mhvk
Copy link
Contributor Author

mhvk commented Apr 14, 2018

@stefanv - oops, that really slipped my mind. I think the sensible way here is to incorporate comments in the PR, so that there is something specific to discuss. I don't think this needs a NEP (we had a long discussion on the mailing list preceding this). As discussed at the top, we might still want to split the docs update (which is not controversial) at all from the PendingDeprecationWarning).

If you have time to take over the PR, please do feel free to use this or just start over.

@mhvk
Copy link
Contributor Author

mhvk commented Apr 25, 2018

Rebased just to see how error messages look (don't seem to get any test failures locally any more, perhaps because of the move to pytest?). Note that this still includes the DOC commit, which I split off to #10973; I'll rebase if that is merged soon (or can remove it here if that helps). EDIT: rebased now that #10973 is merged, so only one commit.

@mhvk mhvk force-pushed the pending-deprecation-for-matrix branch from a84bbbd to 1a7a3ed Compare April 25, 2018 16:03
@mhvk mhvk changed the title DEP,DOC: Pending deprecation warning for matrix DEP: Pending deprecation warning for matrix Apr 25, 2018
@eric-wieser eric-wieser added this to In progress in Matrix deprecation Apr 27, 2018
@charris
Copy link
Member

charris commented May 22, 2018

I think we can ignore that warning in PytestTester and pytest.ini. Adding

# Ignore matrix PendingDeprecationWarning
    ignore:the matrix subclass is not:PendingDeprecationWarning

to pytest.ini fixes the development tests, and adding

            "-W ignore:the matrix subclass is not:PendingDeprecationWarning",

to PytestTester should take care of the release.

Might be able to use module level setup and teardown also, but I don't know that we can trust the warnings module in all the python versions we support.

@charris
Copy link
Member

charris commented May 22, 2018

Might want to rebase on master also.

@@ -70,6 +71,10 @@ class matrix(N.ndarray):
"""
matrix(data, dtype=None, copy=True)

.. note:: It is no longer recommended to use this class, even for linear
algebra. Instead use regular arrays. The class may be removed
in the future.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to get this message into 1.15?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, early enough to deal with the potential fallout in any case. If we follow @rgommers suggestion, latest NumPy will require latest SciPy. We could hold off until the next SciPy release, we are doing that with some f2py changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean #10142 (comment), then I don't think it does (at least, it doesn't matter how the PendingDeprecationWarning is silenced). PendingDeprecationWarnings also won't show up for normal users, only if you run with -Wd or raise exceptions on warnings or something like that.

+1 for getting this note into 1.15

@charris
Copy link
Member

charris commented May 29, 2018

Travis chroot tests have be updated to bionic, so this should be rebased on master.

@mhvk mhvk force-pushed the pending-deprecation-for-matrix branch from 5697572 to dddbb14 Compare May 29, 2018 17:56
@mhvk mhvk force-pushed the pending-deprecation-for-matrix branch from dddbb14 to 86f487c Compare May 29, 2018 17:57
@charris charris merged commit 6721890 into numpy:master May 30, 2018
Matrix deprecation automation moved this from In progress to Done May 30, 2018
@charris
Copy link
Member

charris commented May 30, 2018

Thanks Marten.

@rgommers NumPy will now raise the PendingDeprecationWarning, do what you will with SciPy.

@mhvk mhvk deleted the pending-deprecation-for-matrix branch May 30, 2018 01:04
@mhvk
Copy link
Contributor Author

mhvk commented May 30, 2018

🎆

@charris
Copy link
Member

charris commented Jun 17, 2018

Just to note that I needed pytest 3.6.1, 3.5 was failing.

johnyf added a commit to tulip-control/tulip-control that referenced this pull request Jun 10, 2021
because matrices have been deprecated in `numpy`. Usage of `scipy.sparse` is
causing this issue when conversions to matrices are performed. I changed the:
- calls to the method `scipy.sparse.lil.lil_matrix.todense`
- to calls to the method `scipy.sparse.lil.lil_matrix.toarray`,

to avoid the conversions that raise `PendingDeprecationWarning`s. The warnings
are raised by the call to the method `lil_matrix.todense` because this call
involves the instantiation of the class `numpy.matrix`, which is deprecated.
In contrast, the method `lil_matrix.toarray` creates an instance of
`numpy.ndarray`.

(In fact, in the module `tulip.abstract.discretization`, wherever the method
`lil_matrix.todense` was called, the value that it returned was immediately
converted to a `numpy.ndarray`. So calling the method `lil_matrix.toarray` is
actually more efficient.)

The class `numpy.matrix` is deprecated and will probably be removed in
the future. This will happen after arrangements have been made for
`scipy.space`. (For these points and more information, read the references
listed at the end.)

Still, I do not think that continuing to use `scipy.sparse.lil_matrix` until
when `numpy` removes matrices is a safe approach.
Instead, using `numpy.ndarray` would be safer.
Moreover, I do think that there are other data structures that would fit
this use case better than sparse matrices.


## Diagnosis

I describe below the approach I (eventually) followed to debug this warning,
because finding the cause was difficult.

The issue is a `PendingDeprecationWarning` issued from `numpy`.
This warning is visible in `pytest` runs, but *not* when running the Python
test file directly. Moreover, `pytest` reports the warning, and from which
test function it originates. The warning itself reads (I have wrapped
the lines here):

```
===================================== warnings summary ======================================
abstract_test.py::transition_directions_test
  /.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:69:
  PendingDeprecationWarning: the matrix subclass is not the recommended way to
  represent matrices or deal with linear algebra
  (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html).
  Please adjust your code to use regular ndarray.
    return matrix(data, dtype=dtype, copy=False)
```

So the line in `tulip` that causes the warning cannot be found from the
information contained in the warning.

The above `PendingDeprecationWarning` was introduced in `numpy` in commit:
    numpy/numpy@11e9d2a
This warning was then ignored in the module `scipy.sparse.__init__`, in `scipy` commit:
    scipy/scipy@a874bd5
It appears that this configuration of warnings by `scipy` interacts with
`pytest` complexly:

- when running with `pytest abstract_test.py`,
  the `PendingDeprecationWarning` is visible, but

- when running with `python -X dev -- abstract_test.py`,
  the `PendingDeprecationWarning` is invisible. This behavior is due to
  the call:

  ```python
  warnings.filterwarnings(
      'ignore',
      message='the matrix subclass is not the recommended way')
  ```

  within `scipy.sparse.__init__.py` (introduced in the `scipy` commit
  that was mentioned above). Read also:
  https://docs.python.org/3/library/exceptions.html#PendingDeprecationWarning
  https://www.python.org/dev/peps/pep-0565/

As a result, it is difficult to find the cause within `tulip` of this
`PendingDeprecationWarning`.


## Getting a traceback

The test function that triggered the `PendingDeprecationWarning` from `numpy`
was not failing, so there was no traceback that would indicate which line in
`tulip` caused the warning.

In addition, there was an earlier warning issued by `matplotlib`. So turning
warnings to errors with the argument `-Werror` would cause `pytest` to turn the
`matplotlib` warning into an error, and stop before the warning of interest:

```shell
pytest -Werror abstract_test.py
```

So first I removed the `matplotlib` warnings (temporarily), by commenting the
line `matplotlib.use('Agg')` in the file `abstract_test.py`. This made the
warning of interest to become the first warning. I then passed `-Werror` to
`pytest`, and this turned the `numpy` warning into an error, which produced
the traceback shown below:

(The cause of these `matplotlib` warnings (there are two) is in the package
`polytope`, and has been addressed there, in commit:
    tulip-control/polytope@c464818
These changes will become available to `tulip` with the next `polytope` release.
Until then, the CI tests of `tulip` will raise these `matplotlib` warnings.
These warnings could be explicitly ignored by using `with pytest.warns`.)

(The paths to `tulip` in the traceback below lead to the repository's `tulip`,
instead of a directory under Python's `site-packages`, because during this
phase of debugging I installed `tulip` with `pip install -e .`, to iterate
faster while debugging.)

```
../tulip/abstract/discretization.py:1666: in discretize_switched
    plot_mode_partitions(merged_abstr, show_ts, only_adjacent)
../tulip/abstract/discretization.py:1673: in plot_mode_partitions
    axs = swab.plot(show_ts, only_adjacent)
../tulip/abstract/discretization.py:187: in plot
    ax = ab.plot(show_ts, only_adjacent, color_seed)
../tulip/abstract/discretization.py:403: in plot
    ax = _plot_abstraction(self, show_ts, only_adjacent,
../tulip/abstract/discretization.py:446: in _plot_abstraction
    ax = ab.ppp.plot(
../tulip/abstract/prop2partition.py:600: in plot
    return plot_partition(
.../.virtualenvs/.../lib/python3.9/site-packages/polytope/plot.py:90: in plot_partition
    trans = nx.to_numpy_matrix(trans, nodelist=ppp2trans)
.../.virtualenvs/.../lib/python3.9/site-packages/networkx/convert_matrix.py:553: in to_numpy_matrix
    M = np.asmatrix(A, dtype=dtype)
.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:69: in asmatrix
    return matrix(data, dtype=dtype, copy=False)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

subtype = <class 'numpy.matrix'>
data = array([[1., 0., 0., 0., 0., 0.],
       [0., 1., 1., 0., 1., 1.],
       [1., 0., 1., 1., 0., 1.],
       [1., 0., 0., 1., 0., 0.],
       [0., 0., 0., 0., 1., 1.],
       [1., 0., 0., 0., 0., 1.]])
dtype = None, copy = False

    def __new__(subtype, data, dtype=None, copy=True):
>       warnings.warn('the matrix subclass is not the recommended way to '
                      'represent matrices or deal with linear algebra (see '
                      'https://docs.scipy.org/doc/numpy/user/'
                      'numpy-for-matlab-users.html). '
                      'Please adjust your code to use regular ndarray.',
                      PendingDeprecationWarning, stacklevel=2)
E       PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.

.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:116: PendingDeprecationWarning
```

As the traceback shows, the issue is due to a call to the function
`networkx.to_numpy_matrix` within the function `polytope.plot.plot_partition`.
So avoiding this warning will be possible after the next release of the
package `polytope`.

(Note that inserting an `assert False` in a suitable line within the
function `transition_directions_test` is not an alternative to passing the
argument `-Werror`, because the `assert False` will result in a traceback
where the `assert` statement appears, instead of a traceback that shows the
call stack at the point where the warning was issued.)


## Speeding up debugging using `pytest`

Also, since I had to be running `pytest` on the Python file `abstract_test.py`,
`pytest` would collect all test functions, and run them. The file
`abstract_test.py` happens to contain several slow test functions, so running
them all just to observe the results for the one function of interest is not
time-efficient.

What I did to speed up runs was to rename all `test_*` functions contained in
`abstract_test.py`, except for the one function of interest (namely
`transition_directions_test`), to identifiers outside the patterns collected
by `pytest`.

A simpler alternative, for use with larger test files, is to do the opposite:
rename only the function of interest to a different pattern, and then change
the line `python_functions = ` in the configuration file `pytest.ini`.


## References

- numpy/numpy#10142  (DEP: Pending deprecation warning for matrix)
- numpy/numpy#10973  (DOC: advise against use of matrix)
- scipy/scipy#8887  (MAINT: filter out np.matrix PendingDeprecationWarning's in numpy >=1.15)
- scipy/scipy#9734  (PendingDeprecationWarning for np.matrix with pytest)
- scikit-learn/scikit-learn#12327  (PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices)
- scikit-learn/scikit-learn#13076  ([MRG] Ignore PendingDepWarnings of matrix subclass with pytest)
- cvxpy/cvxpy#567  (NumPy matrix class is pending deprecation and issuing warnings)
- cvxpy/cvxpy#637  (RF: Use a 2D np array instead of matrix to represent scalars)
- cvxpy/cvxpy#638  (RF: Change np.matrix to np.array in several places)
- cvxpy/cvxpy#644  (PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra)
- https://docs.pytest.org/en/6.2.x/warnings.html#deprecationwarning-and-pendingdeprecationwarning
johnyf added a commit to tulip-control/tulip-control that referenced this pull request Jun 20, 2021
because matrices have been deprecated in `numpy`. Usage of `scipy.sparse` is
causing this issue when conversions to matrices are performed. I changed the:
- calls to the method `scipy.sparse.lil.lil_matrix.todense`
- to calls to the method `scipy.sparse.lil.lil_matrix.toarray`,

to avoid the conversions that raise `PendingDeprecationWarning`s. The warnings
are raised by the call to the method `lil_matrix.todense` because this call
involves the instantiation of the class `numpy.matrix`, which is deprecated.
In contrast, the method `lil_matrix.toarray` creates an instance of
`numpy.ndarray`.

(In fact, in the module `tulip.abstract.discretization`, wherever the method
`lil_matrix.todense` was called, the value that it returned was immediately
converted to a `numpy.ndarray`. So calling the method `lil_matrix.toarray` is
actually more efficient.)

The class `numpy.matrix` is deprecated and will probably be removed in
the future. This will happen after arrangements have been made for
`scipy.space`. (For these points and more information, read the references
listed at the end.)

Still, I do not think that continuing to use `scipy.sparse.lil_matrix` until
when `numpy` removes matrices is a safe approach.
Instead, using `numpy.ndarray` would be safer.
Moreover, I do think that there are other data structures that would fit
this use case better than sparse matrices.


## Diagnosis

I describe below the approach I (eventually) followed to debug this warning,
because finding the cause was difficult.

The issue is a `PendingDeprecationWarning` issued from `numpy`.
This warning is visible in `pytest` runs, but *not* when running the Python
test file directly. Moreover, `pytest` reports the warning, and from which
test function it originates. The warning itself reads (I have wrapped
the lines here):

```
===================================== warnings summary ======================================
abstract_test.py::transition_directions_test
  /.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:69:
  PendingDeprecationWarning: the matrix subclass is not the recommended way to
  represent matrices or deal with linear algebra
  (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html).
  Please adjust your code to use regular ndarray.
    return matrix(data, dtype=dtype, copy=False)
```

So the line in `tulip` that causes the warning cannot be found from the
information contained in the warning.

The above `PendingDeprecationWarning` was introduced in `numpy` in commit:
    numpy/numpy@11e9d2a
This warning was then ignored in the module `scipy.sparse.__init__`, in `scipy` commit:
    scipy/scipy@a874bd5
It appears that this configuration of warnings by `scipy` interacts with
`pytest` complexly:

- when running with `pytest abstract_test.py`,
  the `PendingDeprecationWarning` is visible, but

- when running with `python -X dev -- abstract_test.py`,
  the `PendingDeprecationWarning` is invisible. This behavior is due to
  the call:

  ```python
  warnings.filterwarnings(
      'ignore',
      message='the matrix subclass is not the recommended way')
  ```

  within `scipy.sparse.__init__.py` (introduced in the `scipy` commit
  that was mentioned above). Read also:
  https://docs.python.org/3/library/exceptions.html#PendingDeprecationWarning
  https://www.python.org/dev/peps/pep-0565/

As a result, it is difficult to find the cause within `tulip` of this
`PendingDeprecationWarning`.


## Getting a traceback

The test function that triggered the `PendingDeprecationWarning` from `numpy`
was not failing, so there was no traceback that would indicate which line in
`tulip` caused the warning.

In addition, there was an earlier warning issued by `matplotlib`. So turning
warnings to errors with the argument `-Werror` would cause `pytest` to turn the
`matplotlib` warning into an error, and stop before the warning of interest:

```shell
pytest -Werror abstract_test.py
```

So first I removed the `matplotlib` warnings (temporarily), by commenting the
line `matplotlib.use('Agg')` in the file `abstract_test.py`. This made the
warning of interest to become the first warning. I then passed `-Werror` to
`pytest`, and this turned the `numpy` warning into an error, which produced
the traceback shown below:

(The cause of these `matplotlib` warnings (there are two) is in the package
`polytope`, and has been addressed there, in commit:
    tulip-control/polytope@c464818
These changes will become available to `tulip` with the next `polytope` release.
Until then, the CI tests of `tulip` will raise these `matplotlib` warnings.
These warnings could be explicitly ignored by using `with pytest.warns`.)

(The paths to `tulip` in the traceback below lead to the repository's `tulip`,
instead of a directory under Python's `site-packages`, because during this
phase of debugging I installed `tulip` with `pip install -e .`, to iterate
faster while debugging.)

```
../tulip/abstract/discretization.py:1666: in discretize_switched
    plot_mode_partitions(merged_abstr, show_ts, only_adjacent)
../tulip/abstract/discretization.py:1673: in plot_mode_partitions
    axs = swab.plot(show_ts, only_adjacent)
../tulip/abstract/discretization.py:187: in plot
    ax = ab.plot(show_ts, only_adjacent, color_seed)
../tulip/abstract/discretization.py:403: in plot
    ax = _plot_abstraction(self, show_ts, only_adjacent,
../tulip/abstract/discretization.py:446: in _plot_abstraction
    ax = ab.ppp.plot(
../tulip/abstract/prop2partition.py:600: in plot
    return plot_partition(
.../.virtualenvs/.../lib/python3.9/site-packages/polytope/plot.py:90: in plot_partition
    trans = nx.to_numpy_matrix(trans, nodelist=ppp2trans)
.../.virtualenvs/.../lib/python3.9/site-packages/networkx/convert_matrix.py:553: in to_numpy_matrix
    M = np.asmatrix(A, dtype=dtype)
.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:69: in asmatrix
    return matrix(data, dtype=dtype, copy=False)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

subtype = <class 'numpy.matrix'>
data = array([[1., 0., 0., 0., 0., 0.],
       [0., 1., 1., 0., 1., 1.],
       [1., 0., 1., 1., 0., 1.],
       [1., 0., 0., 1., 0., 0.],
       [0., 0., 0., 0., 1., 1.],
       [1., 0., 0., 0., 0., 1.]])
dtype = None, copy = False

    def __new__(subtype, data, dtype=None, copy=True):
>       warnings.warn('the matrix subclass is not the recommended way to '
                      'represent matrices or deal with linear algebra (see '
                      'https://docs.scipy.org/doc/numpy/user/'
                      'numpy-for-matlab-users.html). '
                      'Please adjust your code to use regular ndarray.',
                      PendingDeprecationWarning, stacklevel=2)
E       PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.

.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:116: PendingDeprecationWarning
```

As the traceback shows, the issue is due to a call to the function
`networkx.to_numpy_matrix` within the function `polytope.plot.plot_partition`.
So avoiding this warning will be possible after the next release of the
package `polytope`.

(Note that inserting an `assert False` in a suitable line within the
function `transition_directions_test` is not an alternative to passing the
argument `-Werror`, because the `assert False` will result in a traceback
where the `assert` statement appears, instead of a traceback that shows the
call stack at the point where the warning was issued.)


## Speeding up debugging using `pytest`

Also, since I had to be running `pytest` on the Python file `abstract_test.py`,
`pytest` would collect all test functions, and run them. The file
`abstract_test.py` happens to contain several slow test functions, so running
them all just to observe the results for the one function of interest is not
time-efficient.

What I did to speed up runs was to rename all `test_*` functions contained in
`abstract_test.py`, except for the one function of interest (namely
`transition_directions_test`), to identifiers outside the patterns collected
by `pytest`.

A simpler alternative, for use with larger test files, is to do the opposite:
rename only the function of interest to a different pattern, and then change
the line `python_functions = ` in the configuration file `pytest.ini`.


## References

- numpy/numpy#10142  (DEP: Pending deprecation warning for matrix)
- numpy/numpy#10973  (DOC: advise against use of matrix)
- scipy/scipy#8887  (MAINT: filter out np.matrix PendingDeprecationWarning's in numpy >=1.15)
- scipy/scipy#9734  (PendingDeprecationWarning for np.matrix with pytest)
- scikit-learn/scikit-learn#12327  (PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices)
- scikit-learn/scikit-learn#13076  ([MRG] Ignore PendingDepWarnings of matrix subclass with pytest)
- cvxpy/cvxpy#567  (NumPy matrix class is pending deprecation and issuing warnings)
- cvxpy/cvxpy#637  (RF: Use a 2D np array instead of matrix to represent scalars)
- cvxpy/cvxpy#638  (RF: Change np.matrix to np.array in several places)
- cvxpy/cvxpy#644  (PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra)
- https://docs.pytest.org/en/6.2.x/warnings.html#deprecationwarning-and-pendingdeprecationwarning
johnyf added a commit to tulip-control/tulip-control that referenced this pull request Aug 12, 2021
because matrices have been deprecated in `numpy`. Usage of `scipy.sparse` is
causing this issue when conversions to matrices are performed. I changed the:
- calls to the method `scipy.sparse.lil.lil_matrix.todense`
- to calls to the method `scipy.sparse.lil.lil_matrix.toarray`,

to avoid the conversions that raise `PendingDeprecationWarning`s. The warnings
are raised by the call to the method `lil_matrix.todense` because this call
involves the instantiation of the class `numpy.matrix`, which is deprecated.
In contrast, the method `lil_matrix.toarray` creates an instance of
`numpy.ndarray`.

(In fact, in the module `tulip.abstract.discretization`, wherever the method
`lil_matrix.todense` was called, the value that it returned was immediately
converted to a `numpy.ndarray`. So calling the method `lil_matrix.toarray` is
actually more efficient.)

The class `numpy.matrix` is deprecated and will probably be removed in
the future. This will happen after arrangements have been made for
`scipy.space`. (For these points and more information, read the references
listed at the end.)

Still, I do not think that continuing to use `scipy.sparse.lil_matrix` until
when `numpy` removes matrices is a safe approach.
Instead, using `numpy.ndarray` would be safer.
Moreover, I do think that there are other data structures that would fit
this use case better than sparse matrices.


## Diagnosis

I describe below the approach I (eventually) followed to debug this warning,
because finding the cause was difficult.

The issue is a `PendingDeprecationWarning` issued from `numpy`.
This warning is visible in `pytest` runs, but *not* when running the Python
test file directly. Moreover, `pytest` reports the warning, and from which
test function it originates. The warning itself reads (I have wrapped
the lines here):

```
===================================== warnings summary ======================================
abstract_test.py::transition_directions_test
  /.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:69:
  PendingDeprecationWarning: the matrix subclass is not the recommended way to
  represent matrices or deal with linear algebra
  (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html).
  Please adjust your code to use regular ndarray.
    return matrix(data, dtype=dtype, copy=False)
```

So the line in `tulip` that causes the warning cannot be found from the
information contained in the warning.

The above `PendingDeprecationWarning` was introduced in `numpy` in commit:
    numpy/numpy@11e9d2a
This warning was then ignored in the module `scipy.sparse.__init__`, in `scipy` commit:
    scipy/scipy@a874bd5
It appears that this configuration of warnings by `scipy` interacts with
`pytest` complexly:

- when running with `pytest abstract_test.py`,
  the `PendingDeprecationWarning` is visible, but

- when running with `python -X dev -- abstract_test.py`,
  the `PendingDeprecationWarning` is invisible. This behavior is due to
  the call:

  ```python
  warnings.filterwarnings(
      'ignore',
      message='the matrix subclass is not the recommended way')
  ```

  within `scipy.sparse.__init__.py` (introduced in the `scipy` commit
  that was mentioned above). Read also:
  https://docs.python.org/3/library/exceptions.html#PendingDeprecationWarning
  https://www.python.org/dev/peps/pep-0565/

As a result, it is difficult to find the cause within `tulip` of this
`PendingDeprecationWarning`.


## Getting a traceback

The test function that triggered the `PendingDeprecationWarning` from `numpy`
was not failing, so there was no traceback that would indicate which line in
`tulip` caused the warning.

In addition, there was an earlier warning issued by `matplotlib`. So turning
warnings to errors with the argument `-Werror` would cause `pytest` to turn the
`matplotlib` warning into an error, and stop before the warning of interest:

```shell
pytest -Werror abstract_test.py
```

So first I removed the `matplotlib` warnings (temporarily), by commenting the
line `matplotlib.use('Agg')` in the file `abstract_test.py`. This made the
warning of interest to become the first warning. I then passed `-Werror` to
`pytest`, and this turned the `numpy` warning into an error, which produced
the traceback shown below:

(The cause of these `matplotlib` warnings (there are two) is in the package
`polytope`, and has been addressed there, in commit:
    tulip-control/polytope@c464818
These changes will become available to `tulip` with the next `polytope` release.
Until then, the CI tests of `tulip` will raise these `matplotlib` warnings.
These warnings could be explicitly ignored by using `with pytest.warns`.)

(The paths to `tulip` in the traceback below lead to the repository's `tulip`,
instead of a directory under Python's `site-packages`, because during this
phase of debugging I installed `tulip` with `pip install -e .`, to iterate
faster while debugging.)

```
../tulip/abstract/discretization.py:1666: in discretize_switched
    plot_mode_partitions(merged_abstr, show_ts, only_adjacent)
../tulip/abstract/discretization.py:1673: in plot_mode_partitions
    axs = swab.plot(show_ts, only_adjacent)
../tulip/abstract/discretization.py:187: in plot
    ax = ab.plot(show_ts, only_adjacent, color_seed)
../tulip/abstract/discretization.py:403: in plot
    ax = _plot_abstraction(self, show_ts, only_adjacent,
../tulip/abstract/discretization.py:446: in _plot_abstraction
    ax = ab.ppp.plot(
../tulip/abstract/prop2partition.py:600: in plot
    return plot_partition(
.../.virtualenvs/.../lib/python3.9/site-packages/polytope/plot.py:90: in plot_partition
    trans = nx.to_numpy_matrix(trans, nodelist=ppp2trans)
.../.virtualenvs/.../lib/python3.9/site-packages/networkx/convert_matrix.py:553: in to_numpy_matrix
    M = np.asmatrix(A, dtype=dtype)
.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:69: in asmatrix
    return matrix(data, dtype=dtype, copy=False)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

subtype = <class 'numpy.matrix'>
data = array([[1., 0., 0., 0., 0., 0.],
       [0., 1., 1., 0., 1., 1.],
       [1., 0., 1., 1., 0., 1.],
       [1., 0., 0., 1., 0., 0.],
       [0., 0., 0., 0., 1., 1.],
       [1., 0., 0., 0., 0., 1.]])
dtype = None, copy = False

    def __new__(subtype, data, dtype=None, copy=True):
>       warnings.warn('the matrix subclass is not the recommended way to '
                      'represent matrices or deal with linear algebra (see '
                      'https://docs.scipy.org/doc/numpy/user/'
                      'numpy-for-matlab-users.html). '
                      'Please adjust your code to use regular ndarray.',
                      PendingDeprecationWarning, stacklevel=2)
E       PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.

.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:116: PendingDeprecationWarning
```

As the traceback shows, the issue is due to a call to the function
`networkx.to_numpy_matrix` within the function `polytope.plot.plot_partition`.
So avoiding this warning will be possible after the next release of the
package `polytope`.

(Note that inserting an `assert False` in a suitable line within the
function `transition_directions_test` is not an alternative to passing the
argument `-Werror`, because the `assert False` will result in a traceback
where the `assert` statement appears, instead of a traceback that shows the
call stack at the point where the warning was issued.)


## Speeding up debugging using `pytest`

Also, since I had to be running `pytest` on the Python file `abstract_test.py`,
`pytest` would collect all test functions, and run them. The file
`abstract_test.py` happens to contain several slow test functions, so running
them all just to observe the results for the one function of interest is not
time-efficient.

What I did to speed up runs was to rename all `test_*` functions contained in
`abstract_test.py`, except for the one function of interest (namely
`transition_directions_test`), to identifiers outside the patterns collected
by `pytest`.

A simpler alternative, for use with larger test files, is to do the opposite:
rename only the function of interest to a different pattern, and then change
the line `python_functions = ` in the configuration file `pytest.ini`.


## References

- numpy/numpy#10142  (DEP: Pending deprecation warning for matrix)
- numpy/numpy#10973  (DOC: advise against use of matrix)
- scipy/scipy#8887  (MAINT: filter out np.matrix PendingDeprecationWarning's in numpy >=1.15)
- scipy/scipy#9734  (PendingDeprecationWarning for np.matrix with pytest)
- scikit-learn/scikit-learn#12327  (PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices)
- scikit-learn/scikit-learn#13076  ([MRG] Ignore PendingDepWarnings of matrix subclass with pytest)
- cvxpy/cvxpy#567  (NumPy matrix class is pending deprecation and issuing warnings)
- cvxpy/cvxpy#637  (RF: Use a 2D np array instead of matrix to represent scalars)
- cvxpy/cvxpy#638  (RF: Change np.matrix to np.array in several places)
- cvxpy/cvxpy#644  (PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra)
- https://docs.pytest.org/en/6.2.x/warnings.html#deprecationwarning-and-pendingdeprecationwarning
johnyf added a commit to tulip-control/tulip-control that referenced this pull request Aug 20, 2021
because matrices have been deprecated in `numpy`. Usage of `scipy.sparse` is
causing this issue when conversions to matrices are performed. I changed the:
- calls to the method `scipy.sparse.lil.lil_matrix.todense`
- to calls to the method `scipy.sparse.lil.lil_matrix.toarray`,

to avoid the conversions that raise `PendingDeprecationWarning`s. The warnings
are raised by the call to the method `lil_matrix.todense` because this call
involves the instantiation of the class `numpy.matrix`, which is deprecated.
In contrast, the method `lil_matrix.toarray` creates an instance of
`numpy.ndarray`.

(In fact, in the module `tulip.abstract.discretization`, wherever the method
`lil_matrix.todense` was called, the value that it returned was immediately
converted to a `numpy.ndarray`. So calling the method `lil_matrix.toarray` is
actually more efficient.)

The class `numpy.matrix` is deprecated and will probably be removed in
the future. This will happen after arrangements have been made for
`scipy.space`. (For these points and more information, read the references
listed at the end.)

Still, I do not think that continuing to use `scipy.sparse.lil_matrix` until
when `numpy` removes matrices is a safe approach.
Instead, using `numpy.ndarray` would be safer.


## Diagnosis

I describe below the approach I (eventually) followed to debug this warning,
because finding the cause was difficult.

The issue is a `PendingDeprecationWarning` issued from `numpy`.
This warning is visible in `pytest` runs, but *not* when running the Python
test file directly. Moreover, `pytest` reports the warning, and from which
test function the warning originates. The warning itself reads (I have wrapped
the lines here):

```
===================================== warnings summary ======================================
abstract_test.py::transition_directions_test
  /.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:69:
  PendingDeprecationWarning: the matrix subclass is not the recommended way to
  represent matrices or deal with linear algebra
  (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html).
  Please adjust your code to use regular ndarray.
    return matrix(data, dtype=dtype, copy=False)
```

So the line in `tulip` that causes the warning cannot be found from the
information contained in the warning.

The above `PendingDeprecationWarning` was introduced in `numpy` in commit:
    numpy/numpy@11e9d2a
This warning was then ignored in the module `scipy.sparse.__init__`, in `scipy` commit:
    scipy/scipy@a874bd5
It appears that this configuration of warnings by `scipy` interacts with
`pytest` complexly:

- when running with `pytest abstract_test.py`,
  the `PendingDeprecationWarning` is visible, but

- when running with `python -X dev -- abstract_test.py`,
  the `PendingDeprecationWarning` is invisible. This behavior is due to
  the call:

  ```python
  warnings.filterwarnings(
      'ignore',
      message='the matrix subclass is not the recommended way')
  ```

  within `scipy.sparse.__init__.py` (introduced in the `scipy` commit
  that was mentioned above). Read also:
  https://docs.python.org/3/library/exceptions.html#PendingDeprecationWarning
  https://www.python.org/dev/peps/pep-0565/

As a result, it is difficult to find the cause within `tulip` of this
`PendingDeprecationWarning`.


## Getting a traceback

The test function that triggered the `PendingDeprecationWarning` from `numpy`
was not failing, so there was no traceback that would indicate which line in
`tulip` caused the warning.

In addition, there was an earlier warning issued by `matplotlib`. So turning
warnings to errors with the argument `-Werror` would cause `pytest` to turn the
`matplotlib` warning into an error, and stop before the warning of interest:

```shell
pytest -Werror abstract_test.py
```

So first I removed the `matplotlib` warnings (temporarily), by commenting the
line `matplotlib.use('Agg')` in the file `abstract_test.py`. This made the
warning of interest to become the first warning. I then passed `-Werror` to
`pytest`, and this turned the `numpy` warning into an error, which produced
the traceback shown below:

(The cause of these `matplotlib` warnings (there are two) is in the package
`polytope`, and has been addressed there, in commit:
    tulip-control/polytope@c464818
These changes will become available to `tulip` with the next `polytope` release.
Until then, the CI tests of `tulip` will raise these `matplotlib` warnings.
These warnings could be explicitly ignored by using `with pytest.warns`.)

(The paths to `tulip` in the traceback below lead to the repository's `tulip`,
instead of a directory under Python's `site-packages`, because during this
phase of debugging I installed `tulip` with `pip install -e .`, to iterate
faster while debugging.)

```
../tulip/abstract/discretization.py:1666: in discretize_switched
    plot_mode_partitions(merged_abstr, show_ts, only_adjacent)
../tulip/abstract/discretization.py:1673: in plot_mode_partitions
    axs = swab.plot(show_ts, only_adjacent)
../tulip/abstract/discretization.py:187: in plot
    ax = ab.plot(show_ts, only_adjacent, color_seed)
../tulip/abstract/discretization.py:403: in plot
    ax = _plot_abstraction(self, show_ts, only_adjacent,
../tulip/abstract/discretization.py:446: in _plot_abstraction
    ax = ab.ppp.plot(
../tulip/abstract/prop2partition.py:600: in plot
    return plot_partition(
.../.virtualenvs/.../lib/python3.9/site-packages/polytope/plot.py:90: in plot_partition
    trans = nx.to_numpy_matrix(trans, nodelist=ppp2trans)
.../.virtualenvs/.../lib/python3.9/site-packages/networkx/convert_matrix.py:553: in to_numpy_matrix
    M = np.asmatrix(A, dtype=dtype)
.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:69: in asmatrix
    return matrix(data, dtype=dtype, copy=False)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

subtype = <class 'numpy.matrix'>
data = array([[1., 0., 0., 0., 0., 0.],
       [0., 1., 1., 0., 1., 1.],
       [1., 0., 1., 1., 0., 1.],
       [1., 0., 0., 1., 0., 0.],
       [0., 0., 0., 0., 1., 1.],
       [1., 0., 0., 0., 0., 1.]])
dtype = None, copy = False

    def __new__(subtype, data, dtype=None, copy=True):
>       warnings.warn('the matrix subclass is not the recommended way to '
                      'represent matrices or deal with linear algebra (see '
                      'https://docs.scipy.org/doc/numpy/user/'
                      'numpy-for-matlab-users.html). '
                      'Please adjust your code to use regular ndarray.',
                      PendingDeprecationWarning, stacklevel=2)
E       PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra (see https://docs.scipy.org/doc/numpy/user/numpy-for-matlab-users.html). Please adjust your code to use regular ndarray.

.../.virtualenvs/.../lib/python3.9/site-packages/numpy/matrixlib/defmatrix.py:116: PendingDeprecationWarning
```

As the traceback shows, the issue is due to a call to the function
`networkx.to_numpy_matrix` within the function `polytope.plot.plot_partition`.
So avoiding this warning will be possible after the next release of the
package `polytope`.

(Note that inserting an `assert False` in a suitable line within the
function `transition_directions_test` is not an alternative to passing the
argument `-Werror`, because the `assert False` will result in a traceback
where the `assert` statement appears, instead of a traceback that shows the
call stack at the point where the warning was issued.)


## Speeding up debugging using `pytest`

Also, since I had to be running `pytest` on the Python file `abstract_test.py`,
`pytest` would collect all test functions, and run them. The file
`abstract_test.py` happens to contain several slow test functions, so running
them all just to observe the results for the one function of interest is not
time-efficient.

Running a single test function using `pytest` is possible by writing:

```shell
pytest abstract_test.py::name_of_function
```


## References

- numpy/numpy#10142  (DEP: Pending deprecation warning for matrix)
- numpy/numpy#10973  (DOC: advise against use of matrix)
- scipy/scipy#8887  (MAINT: filter out np.matrix PendingDeprecationWarning's in numpy >=1.15)
- scipy/scipy#9734  (PendingDeprecationWarning for np.matrix with pytest)
- scikit-learn/scikit-learn#12327  (PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices)
- scikit-learn/scikit-learn#13076  ([MRG] Ignore PendingDepWarnings of matrix subclass with pytest)
- cvxpy/cvxpy#567  (NumPy matrix class is pending deprecation and issuing warnings)
- cvxpy/cvxpy#637  (RF: Use a 2D np array instead of matrix to represent scalars)
- cvxpy/cvxpy#638  (RF: Change np.matrix to np.array in several places)
- cvxpy/cvxpy#644  (PendingDeprecationWarning: the matrix subclass is not the recommended way to represent matrices or deal with linear algebra)
- https://docs.pytest.org/en/6.2.x/warnings.html#deprecationwarning-and-pendingdeprecationwarning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants