Kayvan-Zahiri commented on PR #51045:
URL: https://github.com/apache/arrow/pull/51045#issuecomment-5529804074
Understood, and that is your call to make. Happy to close this if you would
rather not
carry it.
One thing worth separating out before I do, because it is independent of the
interchange
question. The existing `test_pandas_roundtrip_categorical` compares the
result to itself:
```python
desc_cat_table = col_result.describe_categorical
desc_cat_result = col_result.describe_categorical
assert desc_cat_table["is_ordered"] == desc_cat_result["is_ordered"]
assert desc_cat_table["is_dictionary"] == desc_cat_result["is_dictionary"]
```
Both sides are `col_result`, so those two assertions hold for any input and
the test cannot
fail on a round-trip mismatch. That is presumably why the dropped `ordered`
flag went
unnoticed. The one-word change to `col_table` on the first line makes it an
actual
comparison.
So there are two things in this PR: the `ordered=` argument in
`categorical_column_to_dictionary`, which sits squarely in the deprecated
protocol, and a
test that silently passes, which does not.
Tell me which you prefer and I will do it:
- Close the whole thing, no hard feelings.
- Keep just the test fix, dropping the `from_dataframe.py` change, so the
assertion is real
for as long as the module is around.
- Merge as is, since it is +7/-4 with the parametrized case already written.
Not looking to spend your time on it either way. I raised it because a test
that cannot
fail seemed worth mentioning regardless of what happens to the protocol.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
