astrofrog/fast-histogram

choice of returned histogram datatype

Open

#5 opened on Jul 19, 2017

View on GitHub
 (9 comments) (0 reactions) (0 assignees)C (28 forks)github user discovery
enhancementhelp wanted

Repository metrics

Stars
 (279 stars)
PR merge metrics
 (PR metrics pending)

Description

Thanks for writing this fast histogram package. A task that we astronomers tend to do often and repeatedly :)

Coming back to the package itself, it seems to me that the return datatype of the histograms are doubles. Conceptually, the histogram is simply a count in that bin, and should be of an integral type. Is the return datatype chosen to be double to reflect the input array type? Or is that in keeping with the numpy convention?

Changing the histogram increment to be an integral type (i.e., the postfix ++ operator) might also lead to better performance. (There are some opportunity for performance improvements -- e.g., pointer operations, reduction of scope for variables, unrolled loops + separate counters)

Separately, any particular reason to perform a bitwise and (&) rather than the logical and (&&) here?

Contributor guide

Research direction
Trace the histogram computation in C code to understand the current use of double for bin counts and evaluate the impact of switching to an integral type. Also review the bitwise AND logic in ` histogram core.c` to confirm correctness.
Tech stack
cpython
Domain
backend
Issue type
Refactor
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
2
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
1-3 hours
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
Active
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
Clear
Prerequisites
C
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
65