TST: Improve tests for numpy.pad #12789
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
and extend coverage to all modes and more variations of a negative stat_length.
Can be grouped with already existing test class checking the behavior for the reflect mode.
Can be grouped in class TestPadWidth as this test checks if an ndarray is accepted as the value to pad_width
These test were ineffective. The TypeError raised in these test was not actually due to pad_width receiving the wrong type but due to the missing parameter mode. Added missing type complex to the appropriate existing test checking for pad_widths type behavior.
lagru
commented
Jan 17, 2019
| @@ -12,6 +12,20 @@ | |||
| from numpy.lib.arraypad import _as_pairs | |||
|
|
|||
|
|
|||
| _all_modes = { | |||
There was a problem hiding this comment.
At some point it made sense to introduce this global variable to avoid duplication.
lagru
commented
Jan 17, 2019
| ((1, 2), (3, 4), (5, 6)), | ||
| ((3, 4, 5), (0, 1, 2)), | ||
| ]) | ||
| @pytest.mark.parametrize("mode", _all_modes.keys()) |
There was a problem hiding this comment.
Using parametrize instead of for loops makes the code more readable but really bloats the number of unit tests and thus the output of pytest. What's the general preference here?
There was a problem hiding this comment.
I think we tend to use parameterize now. If it really does bloat output too much, maybe we can also ask pytest to add some nobs to help.
|
Oh, and reviewing is probably easiest if you follow the commits one by one. Most are independent of another. |
seberg
approved these changes
Feb 12, 2019
| ((1, 2), (3, 4), (5, 6)), | ||
| ((3, 4, 5), (0, 1, 2)), | ||
| ]) | ||
| @pytest.mark.parametrize("mode", _all_modes.keys()) |
There was a problem hiding this comment.
I think we tend to use parameterize now. If it really does bloat output too much, maybe we can also ask pytest to add some nobs to help.
|
@seberg Thanks for looking at this. It's been some time but I remember that there were a few things I still wanted to look at. I'll do so in the next few days and try to finish this. |
Using np.pad instead of directly importing the function seems to be more inline with other test modules.
|
@seberg I think this is ready to go. Test coverage is at 99%. Not covered are:
Also I couldn't figure out how to use |
|
|
mattip
reviewed
Feb 14, 2019
| def test_missing_mode(): | ||
| match = r"pad\(\) missing 1 required positional argument: 'mode'" | ||
| with pytest.raises(TypeError, match=match): | ||
| np.pad(np.ones((5, 6)), 4) |
There was a problem hiding this comment.
tests are failing, it seems the message or exception is different?
There was a problem hiding this comment.
That's actually intended with this test. The problem seems to be that match doesn't match because the returned message leads with _pad_dispatcher() missing 1 required positional [...] instead of pad(). Should I simply change the match-string or is this actually a bug (e.g. _pad_dispatcher not correctly wrapping)? When I try this in my console the returned error leads with pad().
There was a problem hiding this comment.
Just start the match with "missing 1 ...". The _pad_dispatcher is another problem, but out of scope for this PR, pinging @shoyer to take a look
|
@mattip Thanks! It seems like for some reason I need to specify an absolute path for the |
The CLI fails due to error message containing a reference to _pad_dispatcher() being returned instead of pad(). For some reason this test passes when run locally.
|
Thanks @lagru! Nice work. |
|
You're welcome. 🙂 I'll merge these changes into #11358 tomorrow and have a look at what is left to do. |
lagru
added a commit
to lagru/numpy
that referenced
this pull request
Feb 15, 2019
33 tasks
seberg
pushed a commit
that referenced
this pull request
Mar 25, 2019
* ENH: Add support for constant, edge, linear_ramp to new numpy.pad Passes unit tests: - TestConstant - TestEdge - TestZeroPadWidth - TestLegacyVectorFunction - TestNdarrayPadWidth - TestUnicodeInput - TestLinearRamp * MAINT: Simplify diff / change order of functions * MAINT: Revert to old handling of keyword-only arguments * ENH: Add support for stat modes * ENH: Add support for "reflect" mode * MAINT: Remove _slice_column * ENH: Add support for "symmetric" mode * MAINT: Simplify mode "linear_ramp" Creating the linear ramp as an array with 1-sized dimensions except for the one given by `axis` allows implicit broadcasting to the needed shape. This seems to be even a little bit faster that doing this by hand and allows the simplicifaction of the algorithm. Note: Profiling and optimization will be done again at a later stage. * MAINT: Reorder arguments of a sum and fix typo Addresses feedback raised in PR. * ENH: Add support for "wrap" mode This completes the first draft of the complete rewrite meaning all unit tests should pass from this commit onwards. * MAINT: Merge functions for "reflect" and "symmetric" mode The set functions were nearly the same, apart from some index offsets. Merging them reduces code duplication. * TST: Add regression test for gh-11216 The rewrite in past commits fixed this bug. * BUG: Fix edge case for _set_wrap_both when pad_amt contains 0. And include test to protect against regression. * MAINT: Simplify and optimize pad modes Major changes & goals: Don't deal with pad area in the front and back separately. This modularity isn't needed and makes handling of the right edge more awkward. All modes now deal with the left and right side at the same time. Move the creation of the linear ramps fully to its own function which behaves like a vectorized version of linspace. Separate calculation and application of the pad area where possible. This means that _get_edges can be reused for _get_linear_ramps. Combine _normalize_shape and _validate_lengths in a single function which should handles common cases faster. Add new mode "empty" which leaves the padded areas undefined. Add documentation where it was missing. * TST: Don't use np.empty in unit tests * MAINT: Reorder workflow in numpy.pad and deal with empty dimensions Only modes "constant" and "empty" can extend dimensions of size 0. Deal with this edge case gracefully for all other modes either fail or return empty array with padded non-zero dimensions. Handle default values closer to their actual usage. And validate keyword arguments that must be numbers. * MAINT: Add small tweaks to control flow and documentation * BUG: Ensure wrap mode works if right_pad is 0 * ENH: Use reduced region of interest for iterative padding When padding multiple dimensions iteratively corner values are unnecessarily overwritten multiple times. This function reduces the working area for the first dimensions so that corners are excluded. * MAINT: Restore original argument order in _slice_at_axis * MAINT: Keep original error message of broadcast_to * MAINT: Restore old behavior for non-number end_values. * BENCH: Make the pad benchmark pagefault in setup * ENH/TST: Preserve memory layout (order) of the input array and add appropriate unit test. * STY: Revert cosmetical changes to reduce diff * MAINT: Pin dtype to float64 for np.pad's benchmarks * MAINT: Remove redundant code path in _view_roi * MAINT/TST: Provide proper error message for unsupported modes and add appropriate unit test. * STY: Keep docstrings consistent and fix typo. * MAINT: Simplify logical workflow in pad * MAINT: Remove dtype argument from _linear_ramp The responsibility of rounding (but without type conversion) is not really need in _linear_ramp and only makes it a little bit harder to reason about. * DOC: Add version tag to new argument "empty" * MAINT: Default to C-order for padded arrays unless the input is F-contiguous. * MAINT: Name slice of original area consistently for all arguments describing the same thing. * STY: Reduce vertical space * MAINT: Remove shape argument from _slice_at_axis Simplifies calls to this function and the function itself. Using `(...,)` instead should keep this unambiguous. This change is not compatible with Python 2.7 which doesn't support this syntax outside sequence slicing. If that is wanted one could use `(Ellipsis,)` instead. * TST: Test if end_values of linear_ramp are exact which was not given in the old implementation `_arange_ndarray`. * DOC: Improve comments and wrap long line * MAINT: Refactor index_pair to width_pair Calling the right value an index is just plain wrong as it can't be used as such. * MAINT: Make _linear_ramp compatible with size=0 * MAINT: Don't rely on negative indices for slicing Calculating the proper positive index of the start of the right pad area makes it possible to omit the extra code paths for a width of 0. This should make the code easier to reason about. * MAINT: Skip calculation of right_stat if identical If the input area for both sides is the same we don't need to calculate it twice. * TST: Adapt tests from gh-12789 to rewrite of pad * TST: Add tests for mode "empty" * TST: Test dtype persistence for all modes * TST: Test exception for unsupported modes * TST: Test repeated wrapping for each side individually. Reaches some only partially covered if-statments in _set_wrap_both. * TST: Test padding of empty dimension with constant * TST: Test if end_values of linear_ramp are exact which was not given in the old implementation `_arange_ndarray`. (Was accidentally overwritten during the last merge). * TST: Test persistence of memory layout Adapted from an older commit 3ac4d2a which was accidentally overwritten during the last merge. * MAINT: Simplify branching in _set_reflect_both Reduce branching and try to make the calculation of the various indices easier to understand. * TST: Parametrize TestConditionalShortcuts class * TST: Test empty dimension padding for all modes * TST: Keep test parametrization ordered Keep parametrization ordered, otherwise pytest-xdist might believe that different tests were collected during parallelization causing test failures. * DOC: Describe performance improvement of np.pad as well as the new mode "empty" in release notes (see gh-11358). * DOC: Remove outdated / misleading notes These notes are badly worded or actually misleading. For a better explanation on how these functions work have a look at the context and comments just above the lines calling these functions.
grlee77
pushed a commit
to grlee77/numpy
that referenced
this pull request
Mar 26, 2019
* ENH: Add support for constant, edge, linear_ramp to new numpy.pad Passes unit tests: - TestConstant - TestEdge - TestZeroPadWidth - TestLegacyVectorFunction - TestNdarrayPadWidth - TestUnicodeInput - TestLinearRamp * MAINT: Simplify diff / change order of functions * MAINT: Revert to old handling of keyword-only arguments * ENH: Add support for stat modes * ENH: Add support for "reflect" mode * MAINT: Remove _slice_column * ENH: Add support for "symmetric" mode * MAINT: Simplify mode "linear_ramp" Creating the linear ramp as an array with 1-sized dimensions except for the one given by `axis` allows implicit broadcasting to the needed shape. This seems to be even a little bit faster that doing this by hand and allows the simplicifaction of the algorithm. Note: Profiling and optimization will be done again at a later stage. * MAINT: Reorder arguments of a sum and fix typo Addresses feedback raised in PR. * ENH: Add support for "wrap" mode This completes the first draft of the complete rewrite meaning all unit tests should pass from this commit onwards. * MAINT: Merge functions for "reflect" and "symmetric" mode The set functions were nearly the same, apart from some index offsets. Merging them reduces code duplication. * TST: Add regression test for numpygh-11216 The rewrite in past commits fixed this bug. * BUG: Fix edge case for _set_wrap_both when pad_amt contains 0. And include test to protect against regression. * MAINT: Simplify and optimize pad modes Major changes & goals: Don't deal with pad area in the front and back separately. This modularity isn't needed and makes handling of the right edge more awkward. All modes now deal with the left and right side at the same time. Move the creation of the linear ramps fully to its own function which behaves like a vectorized version of linspace. Separate calculation and application of the pad area where possible. This means that _get_edges can be reused for _get_linear_ramps. Combine _normalize_shape and _validate_lengths in a single function which should handles common cases faster. Add new mode "empty" which leaves the padded areas undefined. Add documentation where it was missing. * TST: Don't use np.empty in unit tests * MAINT: Reorder workflow in numpy.pad and deal with empty dimensions Only modes "constant" and "empty" can extend dimensions of size 0. Deal with this edge case gracefully for all other modes either fail or return empty array with padded non-zero dimensions. Handle default values closer to their actual usage. And validate keyword arguments that must be numbers. * MAINT: Add small tweaks to control flow and documentation * BUG: Ensure wrap mode works if right_pad is 0 * ENH: Use reduced region of interest for iterative padding When padding multiple dimensions iteratively corner values are unnecessarily overwritten multiple times. This function reduces the working area for the first dimensions so that corners are excluded. * MAINT: Restore original argument order in _slice_at_axis * MAINT: Keep original error message of broadcast_to * MAINT: Restore old behavior for non-number end_values. * BENCH: Make the pad benchmark pagefault in setup * ENH/TST: Preserve memory layout (order) of the input array and add appropriate unit test. * STY: Revert cosmetical changes to reduce diff * MAINT: Pin dtype to float64 for np.pad's benchmarks * MAINT: Remove redundant code path in _view_roi * MAINT/TST: Provide proper error message for unsupported modes and add appropriate unit test. * STY: Keep docstrings consistent and fix typo. * MAINT: Simplify logical workflow in pad * MAINT: Remove dtype argument from _linear_ramp The responsibility of rounding (but without type conversion) is not really need in _linear_ramp and only makes it a little bit harder to reason about. * DOC: Add version tag to new argument "empty" * MAINT: Default to C-order for padded arrays unless the input is F-contiguous. * MAINT: Name slice of original area consistently for all arguments describing the same thing. * STY: Reduce vertical space * MAINT: Remove shape argument from _slice_at_axis Simplifies calls to this function and the function itself. Using `(...,)` instead should keep this unambiguous. This change is not compatible with Python 2.7 which doesn't support this syntax outside sequence slicing. If that is wanted one could use `(Ellipsis,)` instead. * TST: Test if end_values of linear_ramp are exact which was not given in the old implementation `_arange_ndarray`. * DOC: Improve comments and wrap long line * MAINT: Refactor index_pair to width_pair Calling the right value an index is just plain wrong as it can't be used as such. * MAINT: Make _linear_ramp compatible with size=0 * MAINT: Don't rely on negative indices for slicing Calculating the proper positive index of the start of the right pad area makes it possible to omit the extra code paths for a width of 0. This should make the code easier to reason about. * MAINT: Skip calculation of right_stat if identical If the input area for both sides is the same we don't need to calculate it twice. * TST: Adapt tests from numpygh-12789 to rewrite of pad * TST: Add tests for mode "empty" * TST: Test dtype persistence for all modes * TST: Test exception for unsupported modes * TST: Test repeated wrapping for each side individually. Reaches some only partially covered if-statments in _set_wrap_both. * TST: Test padding of empty dimension with constant * TST: Test if end_values of linear_ramp are exact which was not given in the old implementation `_arange_ndarray`. (Was accidentally overwritten during the last merge). * TST: Test persistence of memory layout Adapted from an older commit 3ac4d2a which was accidentally overwritten during the last merge. * MAINT: Simplify branching in _set_reflect_both Reduce branching and try to make the calculation of the various indices easier to understand. * TST: Parametrize TestConditionalShortcuts class * TST: Test empty dimension padding for all modes * TST: Keep test parametrization ordered Keep parametrization ordered, otherwise pytest-xdist might believe that different tests were collected during parallelization causing test failures. * DOC: Describe performance improvement of np.pad as well as the new mode "empty" in release notes (see numpygh-11358). * DOC: Remove outdated / misleading notes These notes are badly worded or actually misleading. For a better explanation on how these functions work have a look at the context and comments just above the lines calling these functions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This aims to improve the test suite for numpy.pad in anticipation for #11358. E.g. refactor the classes ValueError1, ValueError2, ..., add some missing edge cases and group tests that cover the same aspect of pad. e67ecc3 actually removes ineffective unit tests.