-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Support datetimes in histogram operation #2719
Conversation
Ready to merge when tests pass. |
@@ -204,7 +204,7 @@ def coords(cls, dataset, dim, ordered=False, expanded=False, edges=False): | |||
if edges and not isedges: | |||
data = cls._infer_interval_breaks(data) | |||
elif not edges and isedges: | |||
data = np.convolve(data, [0.5, 0.5], 'valid') | |||
data = data[:-1] + np.diff(data)/2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much simpler than a weird and confusing convolve
call!
holoviews/plotting/mpl/chart.py
Outdated
widths = np.diff(edges) | ||
lims = hist.range(0) + hist.range(1) | ||
return edges[:-1], hist_vals, widths, lims | ||
return edges[:-1], hist_vals, widths, xlim+ylim, datetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of True/False
maybe the more general thing would be to return the type itself (or some more general information about the type than a boolean just about datetimes)? This could allow for other conditional dimension type formatters in future.
If nothing else, grepping for datetime
is a bit of a nightmare so it should be called isdatetime
or similar...
c3a2be0
to
400540e
Compare
Done. |
widths = np.diff(edges) | ||
lims = hist.range(0) + hist.range(1) | ||
return edges[:-1], hist_vals, widths, lims | ||
return edges[:-1], hist_vals, widths, xlim+ylim, isdatetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can still conceive of wanting support for other numeric types one day (e.g rationals?) but this is fine for now. We can use a general type specifier when we find ourselves needing to do something similar for another type. This this fine for now.
Looks good. Merging. |
This PR makes it possible to generate a histogram from datetime data as requested in #2713 and holoviz/hvplot#12. This is consistent with
plt.hist
which also handle datetimes and can be useful to determine the sampling frequency of a dataset.