Fix read_json category dtype #30728
Fix read_json category dtype #30728
Conversation
|
Hello @taoufik07! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-01-06 12:30:52 UTC |
taoufik07
force-pushed
the
fix-read-json-category-dtype
branch
from
55b101f to
8de37e9
Compare
| @@ -892,7 +892,8 @@ def _try_convert_data(self, name, data, use_dtypes=True, convert_dates=True): | |||
| ) | |||
| if dtype is not None: | |||
| try: | |||
| dtype = np.dtype(dtype) | |||
| if not is_categorical_dtype(dtype): | |||
There was a problem hiding this comment.
I don't think you need both the is_categorical_dtype and pandas_dtype here; just the latter should be OK
| @@ -1197,6 +1198,17 @@ def test_read_local_jsonl(self): | |||
| expected = DataFrame([[1, 2], [1, 2]], columns=["a", "b"]) | |||
| tm.assert_frame_equal(result, expected) | |||
|
|
|||
| def test_read_json_category_dtype(self): | |||
There was a problem hiding this comment.
this will need to test all extension dtypes that we support, pls parametrize this over interval, period, categorical, datetime w/tz
There was a problem hiding this comment.
Hey, according to you, what's the best way to deal/pass different input dfs and expected dfs ?
There was a problem hiding this comment.
@taoufik07 have a go using https://docs.pytest.org/en/latest/parametrize.html, there's lots of examples of it being used it other parts of the test suite
There was a problem hiding this comment.
I know about parametrize, but I find having multiple lines (type, input, expected) unpleasant to the eye and I tought that there was some magic function that handles the data generations for all these types, maybe I will just go with the naive way
|
Hi @taoufik07 - sorry to chase you up, just wanted to ask whether you're still working on this :) |
|
Hey @MarcoGorelli, sorry I'm kinda busy right now, yes I would love to keep working on this, I'll try finish the tests in the couple days |
|
Thanks - no hurry, just checking it's not gone stale |
| @@ -892,7 +892,8 @@ def _try_convert_data(self, name, data, use_dtypes=True, convert_dates=True): | |||
| ) | |||
| if dtype is not None: | |||
| try: | |||
| dtype = np.dtype(dtype) | |||
| if not is_categorical_dtype(dtype): | |||
There was a problem hiding this comment.
I don't think you need both the is_categorical_dtype and pandas_dtype here; just the latter should be OK
|
@taoufik07 closing as stale. ping if you want to continue. |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff