TST/CLN: parametrize and clean test_expressions, test_nanops #28553
Conversation
| @@ -152,14 +149,13 @@ def check_fun_data( | |||
| targfunc, | |||
| testarval, | |||
| targarval, | |||
| targarnanval, | |||
There was a problem hiding this comment.
Can you just comment on what this removal is?
There was a problem hiding this comment.
On 212-213 we check if targarnan is None and if so set targarnan = testar. That condition always holds in the test, so ripped it out, and targarnanval as a consequence
| @pytest.mark.parametrize( | ||
| "dtype", | ||
| [ | ||
| np.int16, |
There was a problem hiding this comment.
Not here and no action required but maybe want to expand to cover unsigned and 8 bit versions. I've seen that in a few other PRs so should become a fixture if not already
There was a problem hiding this comment.
we also have our fixtures for dtypes (not blocking this PR)
| np.int64, | ||
| np.float32, | ||
| np.float64, | ||
| getattr(np, "float128", None), |
There was a problem hiding this comment.
Is there a good way to convert this to a skip if not available?
There was a problem hiding this comment.
I guess we could make this:
pytest.param(getattr(np, "float128", None), marks=pytest.skipif(not hasattr(np, "float128"))
Tough call whether the explicit skip is worth the added verbosity
| ) | ||
|
|
||
| def test_nansum(self): | ||
| self.check_funs( | ||
| nanops.nansum, | ||
| np.sum, | ||
| allow_str=False, |
There was a problem hiding this comment.
Is this just universally removed here?
There was a problem hiding this comment.
yah, so just removed allow_str altogether.
| @pytest.mark.parametrize( | ||
| "dtype", | ||
| [ | ||
| np.int16, |
There was a problem hiding this comment.
we also have our fixtures for dtypes (not blocking this PR)
| else: | ||
| assert result.dtype == dtype | ||
| s = Series(range(10), dtype=dtype) | ||
| group_a = ["mean", "std", "var", "skew", "kurt"] |
There was a problem hiding this comment.
likely can be parmaterize using our aggregation fixtures
|
thanks, a couple of follow-on comments |
These are each going to need at least one more pass after this.