fix: Make dx histogram behavior consistent with px#1002
fix: Make dx histogram behavior consistent with px#1002jnumainville merged 18 commits intodeephaven:mainfrom
dx histogram behavior consistent with px#1002Conversation
jnumainville
requested review from
a team and
mattrunyon
and removed request for
a team
| elif self.histnorm == "probability": | ||
| hist_agg_label = f"fraction of sum of {hist_agg_label}" | ||
| elif self.histnorm == "percent": | ||
| hist_agg_label = f"percent of sum of {hist_agg_label}" |
There was a problem hiding this comment.
Why are these X of sum of Y? Looks like this is the case where histfunc is not count or sum. So is there a histfunc that should be required for these cases?
There was a problem hiding this comment.
This is how plotly labels it https://github.com/plotly/plotly.py/blob/master/packages/python/plotly/plotly/express/_core.py#L239
More specifically though, hist_agg_label would contain histfunc at this point.
| pivot_vars: The vars with new column names if a list was passed in | ||
| list_var: The var that was passed in as a list |
There was a problem hiding this comment.
I'm confused by what these do. Is pivot_vars column renames? Is list_var supposed to be something like x or y?
There was a problem hiding this comment.
pivot_vars is kind of a rename - it's for when you pass in a list so the tables need to be stacked with resulting value and variable columns, although they may or may not be named exactly that if those columns already exist in the table. It's not a great name for that dict. I think I'll go ahead and refactor it as part of this.
list_var would be x or y yes, whichever parameter was a list originally
| if self.args.get(self.agg_var): | ||
| self.agg_col: str = ( | ||
| pivot_vars["value"] | ||
| if pivot_vars and list_var and list_var == self.agg_var | ||
| else args[self.agg_var] | ||
| ) | ||
| else: | ||
| # if bar_var is not set, the value column is the same as the axis column | ||
| # because both the axis bins and value are computed from the same inputs | ||
| self.agg_col = self.bin_col |
There was a problem hiding this comment.
Should the comment in the else say agg_var? The if only checks agg_var, not bar_var
3 participants
Fixes: #668
The following examples are now basically consistent.
During testing I discovered #1001 so the legend titles are not set in a couple of these, but that is beyond the scope of this ticket as that is a general plot by issue, not a histogram specific one.
Note a few intentional differences in the args. It's not expected that
dxhistograms andpxhistograms aggregate in the same exact way. Plotly infers the equivalent ofrange_binshence the extra argument. Additionally we havebarmode="group"as our default whereas in Plotly it isbarmode="relative". I believe this was an intentional choice on our end because it's hard to compare the different traces withbarmode="relative"although I don't have a record of it.