Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REF: merge_asof dont use values_for_argsort #45475

Merged
merged 6 commits into from
Jan 25, 2022

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • Ensure all linting tests pass, see here for how to run them
  • whatsnew entry
    cc @mroeschke @jreback

Original motivation here was to remove an anti-pattern usage of _values_for_argsort. In the process I found what I think should be a bug, but I haven't been able to write a test that exposes it, could use some help.

@jbrockmendel jbrockmendel marked this pull request as draft January 19, 2022 20:49
@mroeschke
Copy link
Member

Appears you're trying to tackle joining on nullables from which #32306 would be relevant.

@attack68 attack68 mentioned this pull request Jan 20, 2022
2 tasks
@jbrockmendel jbrockmendel marked this pull request as ready for review January 23, 2022 19:23
@jbrockmendel
Copy link
Member Author

I think this should go in, as even if we don't have a test case that fails on the existing version, the version here i think is reliable. Also this gets rid of the last clearly-antipattern usage of _values_for_argsort

@jbrockmendel jbrockmendel changed the title WIP: merge_asof dont use values_for_argsort REF: merge_asof dont use values_for_argsort Jan 23, 2022
@mroeschke mroeschke added the Refactor Internal refactoring of code label Jan 25, 2022
@mroeschke mroeschke added this to the 1.5 milestone Jan 25, 2022
@@ -1224,6 +1224,49 @@ def test_merge_on_nans(self, func, side):
else:
merge_asof(df, df_null, on="a")

def test_by_nullable(self, any_numeric_ea_dtype):
# Note: this test passes if instead of using pd.array we use
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe note that the outcome of #32306 may be relevant to the expected behavior

@jreback jreback added the ExtensionArray Extending pandas with custom dtypes or arrays. label Jan 25, 2022
@jreback jreback merged commit 1fd31bd into pandas-dev:main Jan 25, 2022
@jbrockmendel jbrockmendel deleted the bug-merge-values_for_argsort branch January 25, 2022 19:29
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ExtensionArray Extending pandas with custom dtypes or arrays. Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants