Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork10.9k
DEP: Deprecate ma.round_ and add warning test#28899
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can run locally to fix the failing tests.
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| ma.round_([1.234, 2.345]) | ||
| assert any("deprecated" in str(warning.message) for warning in w), "No deprecation warning!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Rather than adding here, add to numpy/_core/tests/test_deprecations.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There may also be less boilierplate via i.e,with pytest.deprecated_call(match='deprecated'):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be deleted
…sing pytest.deprecated_call
I’ve removed the separate test_round_warning.py file and moved the test into numpy/core/tests/test_deprecations.py as suggested. |
Please try to build numpy and run the tests locally before pushing to run CI. Using This also needs a release note. |
Thanks for the feedback! 🙌 Will update the PR shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please run the tests locally before pushing a git change.
| Deprecate `ma.round_` function | ||
| ------------------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
RST needs two quotes.
| Deprecate `ma.round_` function | |
| ------------------------------ | |
| Deprecate ``ma.round_`` | |
| ----------------------- |
| with warnings.catch_warnings(record=True) as w: | ||
| warnings.simplefilter("always") | ||
| ma.round_([1.234, 2.345]) | ||
| assert any("deprecated" in str(warning.message) for warning in w), "No deprecation warning!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should be deleted
| @@ -7,6 +7,9 @@ | |||
| import pytest | |||
| import tempfile | |||
| import re | |||
| import numpy.ma as ma | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Linting complains about too many blank lines here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The extra line is causing linting failures. Please fix.
| @@ -8176,8 +8176,13 @@ def round_(a, decimals=0, out=None): | |||
| return out | |||
| round = round_ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
On line 8123 above you need to change the function name fromround_ toround
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You also need to examine and change the documentation to useround instead ofround_. See the Examples section starting on line 8146.
I've addressed all the feedback:
Thank you for your guidance! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should check the reasons CI is failing, and fix them. I pointed out a few, there may be more.
| @@ -7,6 +7,9 @@ | |||
| import pytest | |||
| import tempfile | |||
| import re | |||
| import numpy.ma as ma | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The extra line is causing linting failures. Please fix.
| @@ -8176,8 +8176,13 @@ def round_(a, decimals=0, out=None): | |||
| return out | |||
| round = round_ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You also need to examine and change the documentation to useround instead ofround_. See the Examples section starting on line 8146.
This PR renames the internal function |
Tests are still failing. Also please add the necessary changes to typing. |
Uh oh!
There was an error while loading.Please reload this page.
Deprecate ma.round_ and add warning test