TST: add tests for keeping dtype in Series.update #23604
Conversation
|
Hello @h-vetinari! Thanks for updating the PR.
Comment last updated on November 18, 2018 at 16:17 Hours UTC |
gfyoung
added
Testing
Codecov Report
@@ Coverage Diff @@
## master #23604 +/- ##
=======================================
Coverage 92.28% 92.28%
=======================================
Files 161 161
Lines 51451 51451
=======================================
Hits 47483 47483
Misses 3968 3968
Continue to review full report at Codecov.
|
|
@jreback |
|
@jreback |
|
@jreback |
|
|
||
| expected_values = [caller_values[0], other_values[0], caller_values[2]] | ||
| try: | ||
| # we keep original dtype whenever possible |
There was a problem hiding this comment.
pls don't use a try except inside the test itself. you should explicity use pytest.raises if you want to catch an exception
There was a problem hiding this comment.
@jreback
I know, but this is fully intended (I've made the same argument to @gfyoung in a conversation he marked "resolved" below).
The thing is that Series.update will maintain the dtype of the caller as much as it can. This means, when calling from an int Series, floats get cast to int, if possible. Only if that doesn't work, is the dtype changed to whatever the updated values require.
If I were to pass expected results through the parametrization, I wouldn't be able to parametrize separately, and would therefore need 12 expected results. This is neither easy to read nor extensible.
Please have another look at this test, I think it's quite clear that the try-catch is not about skipping, but about different expected values (which I explained with the comment).
|
|
||
| expected_values = [caller_values[0], other_values[0], caller_values[2]] | ||
| try: | ||
| # we keep original dtype whenever possible |
There was a problem hiding this comment.
@jreback
I know, but this is fully intended (I've made the same argument to @gfyoung in a conversation he marked "resolved" below).
The thing is that Series.update will maintain the dtype of the caller as much as it can. This means, when calling from an int Series, floats get cast to int, if possible. Only if that doesn't work, is the dtype changed to whatever the updated values require.
If I were to pass expected results through the parametrization, I wouldn't be able to parametrize separately, and would therefore need 12 expected results. This is neither easy to read nor extensible.
Please have another look at this test, I think it's quite clear that the try-catch is not about skipping, but about different expected values (which I explained with the comment).
|
@jreback To demonstrate the difference, I've added an explicitly parametrized test with the expected results, and without using a try-catch. You'll see that, in this format, it's actually much much harder to see the underlying logic. And it's just not (reasonably) extensible. I found some minor differences between dtypes, and can easily extend the test with the try-catch (1-line-diff commit coming momentarily). In the demo for the case you want, this would multiply the number of expected results I have to explicitly pass, without a reader being able to grok what's actually happening. |
| ([(61,), (63,)], 'int64', pd.Series([10, (61,), 12])), | ||
| ([(61,), (63,)], float, pd.Series([10., (61,), 12.])), | ||
| ([(61,), (63,)], object, pd.Series([10, (61,), 12])) | ||
| ]) |
There was a problem hiding this comment.
@jreback, this PR is not meant to add the same test twice in different ways, I'm just showing what your review requirement (to avoid try-except) would mean.
There was a problem hiding this comment.
this is a MUCH better test. It is very explicit. remove the other, rename and ping.
|
|
||
| assert_series_equal(s, expected) | ||
|
|
||
| @pytest.mark.parametrize('other_values, caller_dtype, expected', [ |
There was a problem hiding this comment.
call this other, dtype, expected
Ping, as requested. |
|
thanks! |
…fixed * upstream/master: DOC: more consistent flake8-commands in contributing.rst (pandas-dev#23724) DOC: Fixed the doctsring for _set_axis_name (GH 22895) (pandas-dev#22969) DOC: Improve GL03 message re: blank lines at end of docstrings. (pandas-dev#23649) TST: add tests for keeping dtype in Series.update (pandas-dev#23604) TST: For GH4861, Period and datetime in multiindex (pandas-dev#23776) TST: move .str-test to strings.py & parametrize it; precursor to pandas-dev#23582 (pandas-dev#23777) STY: isort tests/scalar, tests/tslibs, import libwindow instead of _window (pandas-dev#23787) BUG: fixed .str.contains(..., na=False) for categorical series (pandas-dev#22170) BUG: Maintain column order with groupby.nth (pandas-dev#22811) API/DEPR: replace kwarg "pat" with "sep" in str.[r]partition (pandas-dev#23767) CLN: Finish isort core (pandas-dev#23765) TST: Mark test_pct_max_many_rows with pytest.mark.single (pandas-dev#23799)
git diff upstream/master -u -- "*.py" | flake8 --diff