Skip to content

REF: axis in [...] becomes _get_axis_number(axis) in Styler #43121

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 30, 2021
Merged

REF: axis in [...] becomes _get_axis_number(axis) in Styler #43121

merged 3 commits into from
Aug 30, 2021

Conversation

yashbg
Copy link
Contributor

@yashbg yashbg commented Aug 20, 2021 •
edited
Loading

@pep8speaks
Copy link

pep8speaks commented Aug 20, 2021 •
edited
Loading

Hello @yashbg! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-08-27 16:42:42 UTC

attack68
attack68 reviewed Aug 20, 2021
View reviewed changes
attack68
attack68 requested changes Aug 20, 2021
View reviewed changes
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.

Thanks - I must have missed this one. small comment.

@yashbg @attack68
Update _apply()
7680143 
Co-authored-by: attack68 <[email protected]>
@yashbg
Copy link
Contributor Author

yashbg commented Aug 20, 2021

Thanks - I must have missed this one. small comment.

I think it was added after you made the changes. Happy to contribute!

@attack68
Copy link
Contributor

LGTM pending green tests

@attack68 attack68 mentioned this pull request Aug 20, 2021
Closed
4 tasks
@attack68
Copy link
Contributor

This gets a mypy error. Not sure why, I think its a curious case of ordering the conditional and it breaks. I've seen this before. See if you can fix it. Failing that can always try adding assert result is DataFrame before the mypy errror.

@MarcoGorelli
Copy link
Member

MarcoGorelli commented Aug 20, 2021 •
edited
Loading

This gets a mypy error. Not sure why, I think its a curious case of ordering the conditional and it breaks. I've seen this before. See if you can fix it. Failing that can always try adding assert result is DataFrame before the mypy errror.

isn't this because func is annotated to return a Styler? (I'll try checking this out later)

@attack68
Copy link
Contributor

Yes good spot. Not sure why mypy didn't complain on previous implementation...

@MarcoGorelli
Copy link
Member

I think it's because apply has its return untyped, so it was being detected as Any

@yashbg
Copy link
Contributor Author

yashbg commented Aug 20, 2021 •
edited
Loading

I think it's because of the change in the ordering of the conditional statements. Earlier the first assignment of result was annotated to be of the type Any, but now it is a Styler, so that might be the reason mypy is causing an error. As mentioned in the mypy docs:

Mypy generally uses the first assignment to a variable to infer the type of the variable.

So one solution would be to change the ordering of the conditional statements again. Or I could annotate the type of result explicitly. Alternatively, I could also use something like a temporary variable to take the output of func() and then typecast it to DataFrame and assign it to result separately, as is done in the if not isinstance(result, DataFrame) block. Which method should I follow?

@attack68
Copy link
Contributor

I would attempt to correct any annotations that might be wrong, its a cleaner and more transparent approach

@yashbg
Copy link
Contributor Author

yashbg commented Aug 20, 2021 •
edited
Loading

I would attempt to correct any annotations that might be wrong, its a cleaner and more transparent approach

The problem is that result is being initialized as a Style but eventually it is being converted to a DataFrame only, which mypy is not able to understand. So I guess explicitly annotating result as a DataFrame could be a reasonable solution.

Otherwise, I think the only annotation which could be changed would be of func(), we could remove the Styler annotation from it, but I'm not sure about this.

@yashbg yashbg closed this Aug 20, 2021
@yashbg yashbg reopened this Aug 20, 2021
@attack68
Copy link
Contributor

lets see how #43148 works in the CI (continuous integration tests). You might be able to merge/update your PR after that

@yashbg
Copy link
Contributor Author

yashbg commented Aug 23, 2021 •
edited
Loading

#43148 seems to have passed the tests. Once it is merged I'll update this PR and hopefully it will work.

@yashbg
Copy link
Contributor Author

yashbg commented Aug 25, 2021

@attack68 will #43148 be merged or should I make those changes in this PR itself?

@MarcoGorelli MarcoGorelli mentioned this pull request Aug 26, 2021
Closed
1 task
@attack68
Copy link
Contributor

@yashbg that PR should be merged, just waiting on its review, but it passes all tests and is green. You can add it here if you like. Either this will then be merged with changes and I close the other, or mine is merged first and when you pull you have same changes so no problems.

@yashbg
Copy link
Contributor Author

yashbg commented Aug 27, 2021

@yashbg that PR should be merged, just waiting on its review, but it passes all tests and is green. You can add it here if you like. Either this will then be merged with changes and I close the other, or mine is merged first and when you pull you have same changes so no problems.

It has been merged. So I'll pull those changes and update my PR.

@yashbg
Copy link
Contributor Author

yashbg commented Aug 27, 2021

The checks seem to have passed. Now the remaining workflows need approval.

@yashbg
Copy link
Contributor Author

yashbg commented Aug 28, 2021

All the checks have finally passed. I think it can be merged now.

MarcoGorelli
MarcoGorelli approved these changes Aug 28, 2021
View reviewed changes
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

@yashbg yashbg requested a review from attack68 August 28, 2021 08:19
@yashbg
Copy link
Contributor Author

yashbg commented Aug 30, 2021

@attack68 could you please review the changes you had requested in the beginning? Then I think the PR can be merged.

attack68
attack68 approved these changes Aug 30, 2021
View reviewed changes
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. thx

@attack68 attack68 merged commit 7472a20 into pandas-dev:master Aug 30, 2021
@attack68 attack68 mentioned this pull request Aug 30, 2021
Closed
3 tasks
@simonjayhawkins simonjayhawkins added the Refactor Internal refactoring of code label Aug 30, 2021
@simonjayhawkins simonjayhawkins added this to the 1.4 milestone Aug 30, 2021
@yashbg yashbg deleted the styler-refactoring branch August 30, 2021 11:42
@simonjayhawkins simonjayhawkins mentioned this pull request Aug 30, 2021
Closed
4 tasks
feefladder pushed a commit to feefladder/pandas that referenced this pull request Sep 7, 2021
@yashbg @attack68 @feefladder
REF: axis in [...] becomes _get_axis_number(axis) in Styler (pand…
b1d12b1 
…as-dev#43121)

* Use _get_axis_number() in _apply()

* Update _apply()

Co-authored-by: attack68 <[email protected]>

Co-authored-by: attack68 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@attack68 attack68 attack68 approved these changes

@MarcoGorelli MarcoGorelli MarcoGorelli approved these changes

Assignees
No one assigned
Labels
Refactor Internal refactoring of code
Projects
None yet
Milestone
1.4
Development

Successfully merging this pull request may close these issues.
5 participants
@yashbg @pep8speaks @attack68 @MarcoGorelli @simonjayhawkins