-
Notifications
You must be signed in to change notification settings - Fork 37
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
ADD: Adding a piechart function to distribution display module. #828
Conversation
This new function uses act.utils.calculate_percentages to create a pie chart of percentages of user defined fields of a dataset.
act/plotting/distributiondisplay.py
Outdated
percent_kwargs : dict | ||
Dictionary of parameters to be used in the act.utils.calculate_percentages | ||
function. None will use default parameters. | ||
|
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.
@zssherman do we need to list kwargs down here in the parameters list?
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.
@zssherman My view on it would be to keep it, just because we are using two kwargs, one for ax.pie and the other for calculate percentages, that way people know where the kwargs have to go depending on which they are using. But I'm open to suggestions
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.
I would lean toward including the kwargs as arguments here, that are passed in. It will be more transparent this way, and we can set the same defaults. So we would need to add
- time
- time_slice
- threshold
- fill_value
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.
Sorry, I was just referring to the documentation and just adding 2 lines to list the kwargs as a parameter. I didn't see it listed in the documentation.
@mgrover1 you're noting that for the percent_kwargs right and not for the kwargs passed to the matplotlib pie function right?
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.
@AdamTheisen There is a mention below that shows it goes to ax.pie, but its more of a sentence then a parameter listing. And yeah Max and I discussed he was thinking for verbose, to add the parameter from calculated_percentages as there is not many and it easier to read and access.
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.
@AdamTheisen I added in the doc the kwargs part to be consistent with the rest of the act docs. Also did the changes @mgrover1 suggested.
This new function uses act.utils.calculate_percentages to create a pie chart of percentages of user defined fields of a dataset.