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

CLN: enforce the deprecation of the Series.argsort NA behavior #58232

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

natmokval
Copy link
Contributor

@natmokval natmokval commented Apr 12, 2024

xref #54219
enforced the deprecation of the Series.argsort NA behavior.

closes #12694
closes #43840

@natmokval natmokval marked this pull request as ready for review April 12, 2024 21:06
"last instead of set to -1.",
FutureWarning,
stacklevel=find_stack_level(),
raise TypeError(
Copy link
Member

Choose a reason for hiding this comment

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

Based on the TODO comment, it appears this should dispatch to self.array.argsort

Also could you see if it closes those associated issues referenced in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the TODO comment, it appears this should dispatch to self.array.argsort

sorry, it’s my mistake, I dispatched Series.argsort to EA.argsort.

Also could you see if it closes those associated issues referenced in the comment?

Yes, it will close issues referenced in the comment.

@mroeschke could you please take a look at this PR? I think CI failures are unrelated to my changes.

Copy link
Member

Choose a reason for hiding this comment

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

cc @jbrockmendel for a double check

res = np.argsort(ser).values
expected = np.argsort(np.array(ser))
tm.assert_numpy_array_equal(res, expected)

# with missing values
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason for deleting this part of the test instead of updating it?

result = pd.Series(data_missing_for_sorting).argsort()
expected = pd.Series(np.array([1, -1, 0], dtype=np.intp))
result = pd.Series(data_missing_for_sorting).argsort()
expected = pd.Series(np.array([2, 0, 1], dtype=np.intp))
Copy link
Member

Choose a reason for hiding this comment

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

are NAs supposed to be sorted to the front or the end? IIUC this is putting them at the front?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REF: Series.argsort should dispatch to EA.argsort BUG: Series.argsort() incorrect handling of NaNs
3 participants