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

Dev #234

Merged
merged 4 commits into from
Jun 1, 2020
Merged

Dev #234

merged 4 commits into from
Jun 1, 2020

Conversation

nsteins
Copy link
Contributor

@nsteins nsteins commented May 20, 2020

This covers an initial implementation of the EventSeries features described in #229

I ended up leaving out the histogram plotting feature as creating reasonable and responsive log binned histograms of time units felt a little outside the scope of this project, though something I may yet tackle.

Would love a review for readability, test coverage, or feature suggestions!

@codecov-commenter
Copy link

codecov-commenter commented May 20, 2020

Codecov Report

Merging #234 into dev will increase coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #234      +/-   ##
==========================================
+ Coverage   87.38%   87.71%   +0.33%     
==========================================
  Files           7        8       +1     
  Lines         856      879      +23     
==========================================
+ Hits          748      771      +23     
  Misses        108      108              
Impacted Files Coverage Δ
traces/__init__.py 100.00% <100.00%> (ø)
traces/eventseries.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cf8c87...8200cf4. Read the comment docs.

@nsteins
Copy link
Contributor Author

nsteins commented May 20, 2020

Ok! I figured it out

@stringertheory
Copy link
Owner

This is awesome, thanks @nsteins!

I'm going to merge this into dev. A couple things this has me mulling over:

  • thinking maybe the events_between should prefix with an n to make it clear that it's returning a count vs. something like a slice. Also, maybe it could follow a similar pattern as the distribution method on TimeSeries of defaulting to total series, and optionally allowing start and end points? There's currently the n_measurements method on TimeSeries — at least I think the naming should be consistent between the two (maybe we could take the opportunity to remove the abbreviation, too — so it could be number_of_measurements and number_of_events_between?
  • Current implementation relies on numpy, but I'd really like to minimize the number of dependencies and keep numpy out of it — it's a lot to depend on for how minimal this package is.
  • Perhaps we could not use an abbreviated method name for cumsum? I realize it's a fairly common abbreviation, but maybe we could take the opportunity to be more explicit with cumulative_sum. Typing (or autocompleting) the extra seven characters seems like it's worth the additional clarity.
  • Would love to have an example in docs/api_reference.rst,
  • I think it would be good to include some tests and documentation around using the EventSeries without passing in all of the data in the constructor (using the add method of the SortedList) so that it's clear that's possible, too.
  • I think time_lag could be renamed interevent_times (that's how it's described in the docstring, and I think i's more clear what to expect). I think it could also be nice to include an interevent_times_distribution that returns a Histogram (and to your comment in Event Based Time Series #229, it would be pretty natural to have plotting methods on the Histogram object).

@stringertheory stringertheory merged commit 8724ab0 into stringertheory:dev Jun 1, 2020
@nsteins
Copy link
Contributor Author

nsteins commented Jul 10, 2020

I made bunch of these changes, will open another PR

I'm going to assign myself an issue for adding documentation and adding a interevent_times_histogram since those need a little more thought

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants