Skip to content

REF: axis in [..] with _get_axis_number() #43151

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

Closed
wants to merge 5 commits into from
Closed

REF: axis in [..] with _get_axis_number() #43151

wants to merge 5 commits into from

Conversation

HardevKhandhar
Copy link

@HardevKhandhar HardevKhandhar commented Aug 21, 2021

Copy link
Contributor

@attack68 attack68 left a comment

Choose a reason for hiding this comment

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

lgtm pending green tests. small comment:

you can convert some of the 4-liners into 1 lines by replacing the pattern:

if axis == 0:
    var = value
else:
    var = other_value

with var = value if axis == 0 else other_value.

If it fits on a single line you can even write:

var = value if frame._get_axis_number(axis) == 0 else other_value

@attack68 attack68 changed the title Axis Args Refactored REF: axis in [..] with _get_axis_number() Aug 21, 2021
@HardevKhandhar
Copy link
Author

Done. Please check.
Thank you.

@jreback jreback added this to the 1.4 milestone Aug 21, 2021
@jreback jreback added the Testing pandas testing functions or related to the test suite label Aug 21, 2021
@attack68
Copy link
Contributor

lgtm pending green

@@ -995,7 +995,8 @@ def test_consistency_for_boxed(box, int_frame_const_col):


def test_agg_transform(axis, float_frame):
other_axis = 1 if axis in {0, "index"} else 0
axis = float_frame._get_axis_number(axis)
other_axis = 1 if axis == 0 else 0
Copy link
Member

Choose a reason for hiding this comment

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

This makes it so we aren't testing cases (e.g. passing "index" to apply). I don't think we want to do this in the tests, only in the pandas code itself.

Copy link
Author

@HardevKhandhar HardevKhandhar Aug 22, 2021

Choose a reason for hiding this comment

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

Changes reverted to original for test_agg_transform function
Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @HardevKhandhar. The same remark applies to your change to the other test here. While the changes you made were certainly of the right form, we just don't want to apply them to the tests. I recommend searching the code in the rest of pandas (outside of tests) and find examples to make this change to.

Copy link
Author

@HardevKhandhar HardevKhandhar Aug 22, 2021

Choose a reason for hiding this comment

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

Alright, I have updated the file to the original code & reverted all changes.
Done. Please Check.
Thanks

Copy link
Member

Choose a reason for hiding this comment

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

If we're making no changes, then I think we can close this PR. But just to be sure - I think you're going in the right direction here, just need to make the modification to cases that occur outside of pandas.tests. Once you find some, make the changes similar to what you did here and submit a new PR.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I am closing the PR.
Thanks a lot @rhshadrach :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants