[WIP] Quick fix to provide complex data type support for hashmap based algorithms #27599
Conversation
… to complex type when sorting is required.
pandas/core/algorithms.py
Outdated
| # simplefilter("ignore", np.ComplexWarning) | ||
| # values = ensure_float64(values) | ||
| # return values, "float64", "float64" | ||
| return ensure_object(np.asarray(values)), "object", "object" |
There was a problem hiding this comment.
we cant just return non-object values? regardless, definitely dont leave the commented-out stuff above, just delete
…x numbers should not be sortable according to test_complex_sorting. The user should specify sort=False when grouping a dataframe by a complex data type.
Codecov Report
@@ Coverage Diff @@
## master #27599 +/- ##
===========================================
- Coverage 93% 42.4% -50.61%
===========================================
Files 182 182
Lines 50311 50312 +1
===========================================
- Hits 46793 21333 -25460
- Misses 3518 28979 +25461
Continue to review full report at Codecov.
|
pandas/tests/test_complex.py
Outdated
| result = algos.value_counts(array) | ||
| tm.assert_series_equal(result, expected) | ||
|
|
||
| @pytest.mark.parametrize("array,expected", [ |
There was a problem hiding this comment.
For any that are just one set of parameters here you can just define in the function body. No need to parametrize
There was a problem hiding this comment.
This is just an initial set of test cases. I plan on adding more test cases for each of the algos.
pandas/tests/test_complex.py
Outdated
| ]) | ||
| def test_factorize(self, array, expected): | ||
| result = pd.factorize(array) | ||
| assert len(result) == 2 |
There was a problem hiding this comment.
I think easier to read at a quick glance if you just compare the two elements rather than the enumerating loop
ghost
changed the title
|
@heckeop can you rebase? the CI failures might have been unrelated problems that have since been fixed |
|
@heckeop is this still active? |
|
Still active. I am on a vacation at the moment, but will be back on this
shortly.
…On Fri, Sep 13, 2019, 5:46 AM William Ayd ***@***.***> wrote:
@heckeop <https://github.com/heckeop> is this still active?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#27599?email_source=notifications&email_token=ALMZCXJZ4HXKSKD2W3QDHBDQJLWHDA5CNFSM4IG7IWU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6TXLGQ#issuecomment-531068314>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ALMZCXIU6WBR2YMTOI62YRDQJLWHDANCNFSM4IG7IWUQ>
.
|
|
@heckeop can you merge master and update |
…rning instead of pytest.warns to capture warnings
|
has numpy undergone a version change? The test below was passing before. Now, the returned object has |
| simplefilter("ignore", np.ComplexWarning) | ||
| values = ensure_float64(values) | ||
| return values, "float64", "float64" | ||
| raise TypeError("Complex data types not supported...Coercing to object") |
There was a problem hiding this comment.
better to do the object-coersion here than fallback to the except branch. clearer
We've made some recent changes to the cython code that handled complex correctly in some cases, not sure if this would be affected. can you merge master and re-push |
|
Thanks for the PR - @heckeop Could you merge master here? |
|
@heckeop can you rebase and see if you can get the CI passing |
|
@heckeop can you merge master |
|
I can maybe take a look if we don’t hear back from @heckeop |
|
Closing as I think this is stale but @heckeop ping if you'd like to pick back up |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff