# Please critique my Histogram.jl implementation

**URL:** <https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614>\
**Category:** New to Julia\
**Created:** [May 9, 2017, 12:39pm UTC](https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614 "2017-05-09T12:39:23Z")\
**Posts on this page:** 7\
**Page:** 1

<div class="post-metadata">

**Author:** ![ndbecker](https://sea2.discourse-cdn.com/julialang/user_avatar/discourse.julialang.org/ndbecker/32/10829_2.png) [@ndbecker](https://discourse.julialang.org/u/ndbecker)\
**Post date:** [May 9, 2017, 12:39pm UTC](https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614/1 "2017-05-09T12:39:23Z")

</div>

I’ve placed beginning of a Histogram implementation here:  
[https://gist.github.com/nbecker/1c02aedd99ed5bfa72bf9284b108f57b](https://gist.github.com/nbecker/1c02aedd99ed5bfa72bf9284b108f57b)

I’d like to hear any suggestions for improving this.  
One thing I’m not sure about is my use of parametrics. Am I overdoing it?

I know that there is a hist.jl in base, but I’m doing this because 1) I want a histogram that behaves differently (behavior on overflow is specified by a parameter, rather than silently ignored), and more because it’s a learning exercise.

Thanks for any feedback.

---

<div class="post-metadata">

**Author:** ![rdeits](https://sea2.discourse-cdn.com/julialang/user_avatar/discourse.julialang.org/rdeits/32/286_2.png) [@rdeits](https://discourse.julialang.org/u/rdeits)\
**Post date:** [May 9, 2017, 4:03pm UTC](https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614/2 "2017-05-09T16:03:44Z")

</div>

A couple of observations:

- Type parameters are generally capitalized by convention in Julia
- Parameterizing the integer type in `cnt_t` is fine, but is it really necessary? Are you planning on using smaller integers than `Int64` for memory conservation or larger integers for overflow issues?
- The type of `ranges::Tuple{Tuple}` is incompletely defined (it matches a tuple with one element, where that element is any kind of tuple). That won’t perform well, and is kind of a strange choice anyway. There’s probably a better choice for that variable. What do you want it to contain?

---

<div class="post-metadata">

**Author:** ![catawbasam](https://sea2.discourse-cdn.com/julialang/user_avatar/discourse.julialang.org/catawbasam/32/2936_2.png) [@catawbasam](https://discourse.julialang.org/u/catawbasam)\
**Post date:** [May 9, 2017, 5:09pm UTC](https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614/3 "2017-05-09T17:09:55Z")

</div>

StatsBase.jl has a Histogram type. See [http://statsbasejl.readthedocs.io/en/latest/empirical.html#histograms](http://statsbasejl.readthedocs.io/en/latest/empirical.html#histograms)

---

<div class="post-metadata">

**Author:** ![ndbecker](https://sea2.discourse-cdn.com/julialang/user_avatar/discourse.julialang.org/ndbecker/32/10829_2.png) [@ndbecker](https://discourse.julialang.org/u/ndbecker)\
**Post date:** [May 9, 2017, 5:36pm UTC](https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614/4 "2017-05-09T17:36:24Z")

</div>

Thanks for the suggestions! I’ve updated the gist. Now it works correctly for N-d.

I’m not really planning to use anything other than Int64, so maybe that parameter is unnecessary.

Yes, I know about StatsBase.jl Histogram. It doesn’t do what I want (overflow is silently ignored), and I wanted to use this as a learning exercise.

Any more suggestions on the new (now corrected) version?

---

<div class="post-metadata">

**Author:** ![ndbecker](https://sea2.discourse-cdn.com/julialang/user_avatar/discourse.julialang.org/ndbecker/32/10829_2.png) [@ndbecker](https://discourse.julialang.org/u/ndbecker)\
**Post date:** [May 11, 2017, 11:53am UTC](https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614/5 "2017-05-11T11:53:26Z")

</div>

I’ve updated my gist [https://gist.github.com/nbecker/1c02aedd99ed5bfa72bf9284b108f57b](https://gist.github.com/nbecker/1c02aedd99ed5bfa72bf9284b108f57b). I added a type (range\_t) to represent the ranges of each axis, hopefully this would be more performant since now all the types in histogram are specified?

Any more suggestions?

---

<div class="post-metadata">

**Author:** ![dpsanders](https://sea2.discourse-cdn.com/julialang/user_avatar/discourse.julialang.org/dpsanders/32/3573_2.png) [@dpsanders](https://discourse.julialang.org/u/dpsanders)\
**Post date:** [May 30, 2017, 1:51pm UTC](https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614/6 "2017-05-30T13:51:52Z")

</div>

I find it more reasonable to use the standard Julia convention `T` for a type parameter, rather than `flt_t`, which feels too C-like.

---

<div class="post-metadata">

**Author:** ![mkborregaard](https://sea2.discourse-cdn.com/julialang/user_avatar/discourse.julialang.org/mkborregaard/32/556_2.png) [@mkborregaard](https://discourse.julialang.org/u/mkborregaard)\
**Post date:** [May 30, 2017, 2:56pm UTC](https://discourse.julialang.org/t/please-critique-my-histogram-jl-implementation/3614/7 "2017-05-30T14:56:22Z")

</div>

Might I also suggest you open an issue/PR on StatsBase for this? The lack of overflow warning is worth discussing.