DEPR: allowing unknown array-likes in merge #48454
Conversation
mroeschke
added
Reshaping
|
Or we could just change it directly for 2.0. This is a pretty weird corner case |
|
Curious if there was a motivating factor from usage in another library or just self discovered? |
|
Self-discovered
…On Fri, Sep 9, 2022 at 10:24 AM Matthew Roeschke ***@***.***> wrote:
Curious if there was a motivating factor from usage in another library or
just self discovered?
—
Reply to this email directly, view it on GitHub
<#48454 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB5UM6B7K6NNVYPHQPLPTF3V5NXFFANCNFSM6AAAAAAQHHFRLA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Up to you how strongly you feel about avoiding this in 2.0. Personally, I think we should use the 2.0 hard break for functionality that would be a PITA to deprecate normally. |
Not the end of the world to wait until 3.0 |
|
Cool, then a whatsnew note in 1.6 would probably be good. But hopefully we can do the 1.x deprecation cleaning spree before adding deprecations in 2.0. |
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
|
||
| msg = "will only allow array-like objects that are one of the following types" | ||
| with tm.assert_produces_warning(FutureWarning, match=msg): | ||
| res = merge(left, right, left_on="A", right_on=arr) |
There was a problem hiding this comment.
Could you also add tests that the FutureWarning shows up when the dask array is passed to left_on and on?
There was a problem hiding this comment.
huh this now raises for me locally; i wonder if it isnt getting run on the CI? ive gotta run, will dig into this later
|
This now raises in Int64Factorizer when I pass in a dask.array (in the test this currently adds). In 1.5.3 passing a dask array worked for My initial motivation here was to be able to accurately annotate the merge code, which ATM uses is_array_like but doesn't actually require "ndarray | EA | Series | Index". Seeing as how the API has already been changed, any objection to making that change explicit+documented by checking for the specific types? |
|
I'd be ok with that. Fixing wouldn't be too hard I guess (we called ensure_...) before, but not sure if it's worth it |
|
Closing, will address in a few months once we confirm no one complains about the changed behavior. |
Not sure what the current status is w/r/t new deprecations.
Until we make this change, some of our annotations in this module are lies.