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

Histogram enhancements #2812

Merged
merged 4 commits into from Jun 22, 2018

Conversation

Projects
None yet
3 participants
@philippjfr
Copy link
Contributor

philippjfr commented Jun 19, 2018

Various enhancements for the histogram operation:

  • Remove individually parameter which didn't do anything
  • Added cumulative parameter to compute cumulative histogram (#2811)
  • Added option to pass in explicit list/array of bins
  • Adds unit tests
@@ -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="""

This comment has been minimized.

@jlstevens

jlstevens Jun 20, 2018

Contributor

What about tuples or pandas series? Do we want to support lots of different types or force a single type?

This comment has been minimized.

@philippjfr

philippjfr Jun 20, 2018

Author Contributor

I'd be okay adding tuples I suppose, although that doesn't seem particularly common.

This comment has been minimized.

@jlstevens

jlstevens Jun 20, 2018

Contributor

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.

This comment has been minimized.

@philippjfr

philippjfr Jun 20, 2018

Author Contributor

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.

This comment has been minimized.

@jbednar

jbednar Jun 20, 2018

Contributor

You're not sure it makes sense?

This comment has been minimized.

@philippjfr

philippjfr Jun 20, 2018

Author Contributor

Right, fixed.

This comment has been minimized.

@jlstevens

jlstevens Jun 22, 2018

Contributor

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.

This comment has been minimized.

@jlstevens

jlstevens Jun 22, 2018

Contributor

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.

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jun 20, 2018

Made one comment. Otherwise looks good.

@philippjfr philippjfr force-pushed the histogram_enhancements branch from 0edd7aa to 8b870d5 Jun 20, 2018

@jlstevens

This comment has been minimized.

Copy link
Contributor

jlstevens commented Jun 22, 2018

Looks good. Merging.

@jlstevens jlstevens merged commit eda6350 into master Jun 22, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 83.026%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the histogram_enhancements branch Jul 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.