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

Fix NumPy deprecation warning #289

Merged
merged 2 commits into from
Oct 28, 2021
Merged

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Jul 6, 2021

Motivation and context:

See 1 for details in the release notes. Using np.floating will
capture more types than np.float, but is arguably closer to what one
would expect from an is_float function and consistent with the
is_integer function.

Interactions with other PRs:

none

How has this been tested?

existing tests

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • [n/a] I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

I think this change does not need a changelog entry as it shouldn't affect the public API, but if desired I can add one.

@jgosmann jgosmann requested review from tbekolay and Seanny123 July 6, 2021 14:17
@jgosmann jgosmann added this to the 1.3.0 milestone Jul 6, 2021
@tbekolay
Copy link
Member

tbekolay commented Jul 7, 2021

I think np.floating is not meant to be used this way; a few things I tried in the terminal:

>>> isinstance(1.0, np.floating)
False
>>> isinstance(np.array(1.0), np.floating)
False
>>> isinstance(np.array([1.0]), np.floating)
False

Probably to make this work we'll need to check if it is indeed a NumPy array first, then check the dtype; e.g.

>>> np.issubdtype(np.array([1.0]).dtype, np.floating)
True

This might do the same thing, and seems a bit more obvious:

>>> np.array([1.0]).dtype.kind == "f"
True

Edit: This might mean that our is_int implementation is wrong and should be fixed as well, I didn't look back at it.

@jgosmann jgosmann self-assigned this Jul 7, 2021
@jgosmann jgosmann force-pushed the fix-np-typecheck-deprecation branch from ee7b3c0 to 0bbcf4d Compare July 25, 2021 13:23
@jgosmann
Copy link
Collaborator Author

I updated the PR. This probably needs to be fixed in Nengo core too.

@jgosmann jgosmann force-pushed the fix-np-typecheck-deprecation branch 4 times, most recently from aa4b1a0 to a3e12e9 Compare July 26, 2021 20:08
@jgosmann jgosmann removed their assignment Jul 27, 2021
@jgosmann
Copy link
Collaborator Author

Finally got a green build. :)

These methods were not working correctly for scalar arrays like
`np.array(42)` because the `isinstance` check on, for example,
`np.integer` was not correct.

This also fixes a deprecation warning See [1] for details in the release
notes. Using `np.floating` will capture more types than np.float, but is
arguably closer to what one would expect from an `is_float` function and
consistent with the `is_integer` function.

[1]: https://numpy.org/devdocs/release/1.20.0-notes.html#using-the-aliases-of-builtin-types-like-np-int-is-deprecated
@jgosmann jgosmann force-pushed the fix-np-typecheck-deprecation branch from a3e12e9 to ac1edb0 Compare October 28, 2021 15:59
@jgosmann jgosmann merged commit 1d0914f into master Oct 28, 2021
@jgosmann jgosmann deleted the fix-np-typecheck-deprecation branch October 28, 2021 16:41
jgosmann added a commit that referenced this pull request Oct 28, 2021
I forgot to commit this file as part of d3712e0
(PR #289).
@jgosmann jgosmann mentioned this pull request Oct 28, 2021
4 tasks
jgosmann added a commit that referenced this pull request Oct 28, 2021
I forgot to commit this file as part of d3712e0
(PR #289).
jgosmann added a commit that referenced this pull request Oct 29, 2021
I forgot to commit this file as part of
d3712e0 (PR #289).
jgosmann added a commit that referenced this pull request Oct 29, 2021
I forgot to commit this file as part of
d3712e0 (PR #289).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants