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.

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

VIS: Accept xlabel and ylabel for scatter and hexbin plots #37102

Merged
merged 2 commits into from
Oct 14, 2020
Merged

VIS: Accept xlabel and ylabel for scatter and hexbin plots #37102

merged 2 commits into from
Oct 14, 2020

Conversation

treuherz
Copy link
Contributor

@treuherz
Copy link
Contributor Author

I'm a bit puzzled by the type annotations on these params (from #34827)

pandas/pandas/plotting/_matplotlib/core.py

Lines 105 to 106 in 9cb3723

xlabel: Optional[Label] = None,
ylabel: Optional[Label] = None,

They're Optional[Label] but Label = Optional[Hashable]. I'm all for making the Optional-ness of the parameters explicit but I'm not sure if the nesting has any side-effects, or why Label is Optional in the first place.

@charlesdong1991
Copy link
Member

charlesdong1991 commented Oct 13, 2020 •
edited
Loading

I'm not sure if the nesting has any side-effects

emm, any comments on the "potential" side-effect it might have? @simonjayhawkins I added to make it more explicit, probably also okay if only having xlabel: Label = None?

@charlesdong1991 charlesdong1991 added this to the 1.2 milestone Oct 13, 2020
charlesdong1991
charlesdong1991 approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member

@charlesdong1991 charlesdong1991 left a comment

Choose a reason for hiding this comment

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

very nice!

only one minor comment

pandas/tests/plotting/test_frame.py Show resolved Hide resolved
@jreback
Copy link
Contributor

jreback commented Oct 14, 2020

lgtm. @charlesdong1991 merge when updated / green.

@simonjayhawkins
Copy link
Member

emm, any comments on the "potential" side-effect it might have? @simonjayhawkins I added to make it more explicit, probably also okay if only having xlabel: Label = None?

not an issue. adding Optional is fine. Where the problem occurs is where we need to distinguish between user passing None as a valid label, I don't think that's relevant here.

from pandas._typing import Label
from typing import Optional

a: Optional[Label]
reveal_type(a)  # Revealed type is 'Union[typing.Hashable, None]'

@treuherz
Copy link
Contributor Author

Not sure why that check failed, it's way outside this PR

@charlesdong1991
Copy link
Member

hmm, this seems to be related to #37024

will let you know once it's fixed so that you could rebase, sorry @treuherz

@simonjayhawkins
Copy link
Member

hmm, this seems to be related to #37024

will let you know once it's fixed so that you could rebase, sorry @treuherz

cc @arw2019 from changes in #37050

@charlesdong1991
Copy link
Member

charlesdong1991 commented Oct 14, 2020 •
edited
Loading

OK @simonjayhawkins shall i revert that PR to get CI green for now?

ah, just saw you have done that, sorry for the noise

@charlesdong1991 charlesdong1991 merged commit e4e1b63 into pandas-dev:master Oct 14, 2020
@charlesdong1991
Copy link
Member

very nice! congrats on your first PR on pandas! @treuherz

@treuherz treuherz deleted the fix_issue_37001 branch October 15, 2020 08:02
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Oct 26, 2020
@treuherz @JulianWgs
VIS: Accept xlabel and ylabel for scatter and hexbin plots (pandas-de…
a25b37c 
…v#37102)

* VIS: Accept xlabel and ylabel for scatter and hexbin plots

* Add issue number note
kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request Nov 2, 2020
@treuherz
VIS: Accept xlabel and ylabel for scatter and hexbin plots (pandas-de…
7905f21 
…v#37102)

* VIS: Accept xlabel and ylabel for scatter and hexbin plots

* Add issue number note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlesdong1991 charlesdong1991 charlesdong1991 approved these changes

Assignees
No one assigned
Labels
Enhancement Visualization plotting
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.

VIS: Allow xlabel and ylabel in DataFrame.plot.scatter plot and hexbin plot
4 participants
@treuherz @charlesdong1991 @jreback @simonjayhawkins