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

Display both absolute frequencies and percentages #89

Merged
merged 12 commits into from Mar 17, 2020

Conversation

@maziello
Copy link
Contributor

maziello commented Feb 5, 2020

Closes #77

Added option to display both the absolute frequency and the percentage of the total for each intersection and category.
Refactored the string formatting using .format() instead of %

maziello added 6 commits Feb 5, 2020
…e of

  the total for each intersection and category.
Refactored the string formatting using .format() instead of %
@jnothman

This comment has been minimized.

Copy link
Owner

jnothman commented Feb 5, 2020

Thanks for the pull request. @arosenfeld addressees the same issue in #77. Unfortunately I've not found the time and focus to touch that up and merge it. There were some tricky questions of consistency there. I would appreciate if you took a look at that pull request and compare how it handles this matter.

@maziello

This comment has been minimized.

Copy link
Contributor Author

maziello commented Feb 6, 2020

Thanks for the quick response. I have looked into issue #77 and I like the idea of adding a new parameter. I think show_counts and show_percents should not affect each other. What I mean is we can still keep the flexibility of the current show_counts implementation and display percentages on top of it.
I hope the examples below help:
Default plots have no labels:

import upsetplot
 
example = upsetplot.generate_counts()
plot(example, show_counts=False, show_percents=False)

upset_no_labels

Percentages can be displayed even with no counts:

plot(example, show_counts=False, show_percents=True)

upset_percents_only

When counts and percentages are displayed, percentages are in a new line below:

plot(example, show_counts=True, show_percents=True)

upset_percents_counts

This strategy keeps the current formatting functionality of show_counts, with or without displaying percentages:

plot(example, show_counts='%.2f', show_percents=True)

upset_percents_counts_floats

The change only affects the label generation, adding the new parameter and doesn't break any existing implementation.
What do you think?

maziello added 3 commits Feb 7, 2020
@maziello maziello requested a review from jnothman Feb 22, 2020
@jnothman

This comment has been minimized.

Copy link
Owner

jnothman commented Feb 24, 2020

@jnothman jnothman mentioned this pull request Mar 15, 2020
@jnothman

This comment has been minimized.

Copy link
Owner

jnothman commented Mar 15, 2020

Hi @maziello, I've tried to refactor this and add some tests. WDYT?

@jnothman

This comment has been minimized.

Copy link
Owner

jnothman commented Mar 15, 2020

Hi @arosenfeld, in the end i'm inclining to adopt this version. Let me know your thoughts.

@arosenfeld

This comment has been minimized.

Copy link
Contributor

arosenfeld commented Mar 15, 2020

Hi @jnothman, I haven't had a chance to look at the code itself from @maziello, but the plots themselves look great. Go for it!

@maziello

This comment has been minimized.

Copy link
Contributor Author

maziello commented Mar 16, 2020

Hi @jnothman, it looks great. Go for it!
Thanks a lot for looking into this!!!

@jnothman jnothman merged commit 8018efc into jnothman:master Mar 17, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+5.3%) to 98.044%
Details
@jnothman

This comment has been minimized.

Copy link
Owner

jnothman commented Mar 17, 2020

Thank you @maziello and @arosenfeld!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.