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

MAINT: Deprecate numpy matrix conversion functions #4238

Merged
merged 5 commits into from
Oct 7, 2020

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented Oct 5, 2020

Adds deprecation warnings + associated tests to to_numpy_matrix and from_numpy_matrix.

Originally, from_numpy_array as simply a pass-through function calling from_numpy_matrix. Re-organized so that from_numpy_matrix is now the pass-through, and the implementation is in from_numpy_array to make the removal easier down the road.

Added tests to ensure the deprecation warnings are raised, and warnings filters to suppress warnings related to to/from_numpy_matrix in the remainder of the test suite.

@rossbar rossbar added this to the networkx-2.6 milestone Oct 5, 2020
networkx/conftest.py Outdated Show resolved Hide resolved
@jarrodmillman
Copy link
Member

Could you a note in both:

  • doc/release/release_dev.rst
  • doc/developer/deprecations.rst

@jarrodmillman
Copy link
Member

@rossbar I merged #4235. Could you rebase on master when you have a chance?

@jarrodmillman
Copy link
Member

It looks like you will manually need to redo 0d0ebe0, since it is keeping the reformatted text and black is fine w/ it either way. I prefer the shorter lines here and the reformatting isn't relevant for this PR.

@rossbar
Copy link
Contributor Author

rossbar commented Oct 6, 2020

It looks like you will manually need to redo 0d0ebe0, since it is keeping the reformatted text and black is fine w/ it either way. I prefer the shorter lines here and the reformatting isn't relevant for this PR.

The style from 0d0ebe0 is the one that the current version of black (20.8b1) gives me. I overrode it manually and pushed in df7e11a, but that fails CI (it fails for me locally with the pre-commit checks too).

networkx/conftest.py Outdated Show resolved Hide resolved
networkx/conftest.py Outdated Show resolved Hide resolved
@networkx networkx deleted a comment from pep8speaks Oct 7, 2020
@pep8speaks
Copy link

pep8speaks commented Oct 7, 2020

Hello @rossbar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 540:80: E501 line too long (82 > 79 characters)
Line 541:80: E501 line too long (87 > 79 characters)
Line 648:80: E501 line too long (84 > 79 characters)
Line 653:80: E501 line too long (88 > 79 characters)
Line 1273:80: E501 line too long (87 > 79 characters)
Line 1338:80: E501 line too long (85 > 79 characters)

Comment last updated at 2020-10-07 01:46:40 UTC

@jarrodmillman
Copy link
Member

jarrodmillman commented Oct 7, 2020

This LGTM. Two final minor suggestions:

  1. In convert_matrix.py in the example for from_numpy_matrix we can change
    >>> import numpy as np
    >>> A = np.array([[1, 1], [2, 1]])
    >>> G = nx.from_numpy_array(A)
    >>> G.edges(data=True)
    EdgeDataView([(0, 0, {'weight': 1}), (0, 1, {'weight': 2}), \
(1, 1, {'weight': 1})])

to

    >>> import numpy as np
    >>> A = np.array([[1, 1], [2, 1]])
    >>> G = nx.from_numpy_array(A)
    >>> G.edges(data=True)
    EdgeDataView([(0, 0, {'weight': 1}), (0, 1, {'weight': 2}), (1, 1, {'weight': 1})])

now that we allow 88 characters per line.

  1. Also in from_numpy_matrix,
    # This should never fail if you have created a numpy array with numpy...
    import numpy as np

(This was added years ago.) Should we remove those two lines, since np isn't used in the function?

@rossbar
Copy link
Contributor Author

rossbar commented Oct 7, 2020

Yup - good suggestions! I also un-special-cased a line in the dtype-to-pytype dictionary.

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

This looks good to me. :}

@jarrodmillman jarrodmillman merged commit ddf707d into networkx:master Oct 7, 2020
@rossbar rossbar deleted the dep/convert_numpy_matrix branch October 17, 2020 17:39
MridulS pushed a commit to MridulS/networkx that referenced this pull request Feb 4, 2023
* DEP: Deprecate to/from_numpy_matrix.

Move implementation of from_numpy_matrix to
from_numpy_array.

* TST: Add tests and warnings filters for deprecations.

* DOC: Add deprecation notes.

* STY: fix autofmt'd lines in conftest.

* MAINT: apply suggestions from code review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants