-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
fix: support non-string groupbys for pie chart #10493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Than's for the quick and very comprehensive fix!
tests/viz_tests.py
Outdated
base_df = pd.DataFrame( | ||
data={ | ||
"intcol": [1, 2, 3, 4], | ||
"floatcol": [0.1, 0.2, 0.3, 0.4], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it help to insert a couple of Nones in these columns as well? And maybe add a boolean column, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, numeric null
s were not properly handled (showed up as nan
), adding tests.
ac69124
to
7a08776
Compare
7a08776
to
e40cf91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good!
* chore: add unit tests to pie chart * refine logic for floats and nans and add more tests
* chore: add unit tests to pie chart * refine logic for floats and nans and add more tests
* chore: add unit tests to pie chart * refine logic for floats and nans and add more tests
* chore: add unit tests to pie chart * refine logic for floats and nans and add more tests
* chore: add unit tests to pie chart * refine logic for floats and nans and add more tests
* chore: add unit tests to pie chart * refine logic for floats and nans and add more tests
* chore: add unit tests to pie chart * refine logic for floats and nans and add more tests
SUMMARY
This adds support for non-string labels to the Pie Chart Viz and uses
NULL_STRING
if a pie label isNone
. Also removes decimals for float groupbys that satisfyis_integer()
. This is needed, as allint
columns becomefloat
if they containNULL
s (Pandas limitation).BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
CI + new tests + local testing
ADDITIONAL INFORMATION