Histogram enhancements #2812
Histogram enhancements #2812
Conversation
philippjfr
added
the
type: enhancement
holoviews/operation/element.py
Outdated
| @@ -486,6 +486,12 @@ class histogram(Operation): | |||
| bin_range = param.NumericTuple(default=None, length=2, doc=""" | |||
| Specifies the range within which to compute the bins.""") | |||
|
|
|||
| bins = param.ClassSelector(default=None, class_=(np.ndarray, list), doc=""" | |||
There was a problem hiding this comment.
What about tuples or pandas series? Do we want to support lots of different types or force a single type?
There was a problem hiding this comment.
I'd be okay adding tuples I suppose, although that doesn't seem particularly common.
There was a problem hiding this comment.
We should use a known set of types for 'list-like' types. Could you open an issue about this? Doesn't need to be fixed in this PR.
There was a problem hiding this comment.
After considering this I'm not sure this makes sense tbh. It's simply too dependent on the particular use case. In many cases ndarray and lists might be used interchangeably but often supporting arrays doesn't really make sense. Then there's lists and tuples, which again can be used interchangeably in some cases but are treated completely differently in others.
I've now added tuples here, but I don't think that generalizes in any real sense.
There was a problem hiding this comment.
You're not sure it makes sense?
There was a problem hiding this comment.
My (not particularly strongly held) opinion is that it should only support lists: ndarray depends on numpy (not a built-in) and as Philipp points out we have cases where we treat tuples and lists differently. For parameters we would often enforce lists with param.List instead of a class selector which is what would have probably made more sense here.
There was a problem hiding this comment.
Anyway, this is a minor point which we can decide on later and fix in a subsequent PR if we want to. I'll go ahead and merge.
|
Made one comment. Otherwise looks good. |
philippjfr
force-pushed
the
histogram_enhancements
branch
from
0edd7aa
to
8b870d5
Compare
|
Looks good. Merging. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Various enhancements for the histogram operation:
individuallyparameter which didn't do anythingcumulativeparameter to compute cumulative histogram (Feature request: cumulative hist #2811)