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.

Sign up for GitHub

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

Jump to bottom

recovery: windowed min and max filter implementation #398

Merged
merged 1 commit into from
Mar 16, 2020
Merged

recovery: windowed min and max filter implementation #398

merged 1 commit into from
Mar 16, 2020

Conversation

lohith-bellad
Copy link
Contributor

Port google opensourced min-max filter code based on
Kathleen Nichols' algorithm for tracking the minimum
(or maximum) value of a data stream over some fixed
time interval.

@lohith-bellad lohith-bellad requested a review from a team as a code owner March 7, 2020 00:16
LPardue
LPardue reviewed Mar 7, 2020
View reviewed changes
Copy link
Contributor

@LPardue LPardue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started making some comments but then as I worked through the code, it seems like its design is made really hard by MinmaxSample::time being an Option. No code seems to care about the None case, it just unwraps the Option which can result in a panic. All the test cases seem to require the reset method to be used which initializes the time. I think it would be easier to lose the option and make a Minmax::new() method that is used to initialize everything

src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
junhochoi
junhochoi reviewed Mar 7, 2020
View reviewed changes
src/recovery.rs Outdated Show resolved Hide resolved
src/recovery.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
@lohith-bellad lohith-bellad force-pushed the windowed_minmax_filter branch 3 times, most recently from f806c43 to 6895d5c Compare March 8, 2020 20:44
@lohith-bellad
Copy link
Contributor Author

Addressed all the comments. Please review, thanks

LPardue
LPardue reviewed Mar 8, 2020
View reviewed changes
src/minmax.rs Show resolved Hide resolved
ghedo
ghedo requested changes Mar 9, 2020
View reviewed changes
Copy link
Member

@ghedo ghedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments.

src/minmax.rs Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/recovery.rs Outdated Show resolved Hide resolved
@lohith-bellad lohith-bellad force-pushed the windowed_minmax_filter branch 3 times, most recently from 95356b6 to d650b97 Compare March 9, 2020 19:40
junhochoi
junhochoi previously approved these changes Mar 10, 2020
View reviewed changes
Copy link
Contributor

@junhochoi junhochoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@junhochoi
Copy link
Contributor

This will close #305

ghedo
ghedo requested changes Mar 13, 2020
View reviewed changes
Copy link
Member

@ghedo ghedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good now, added a few more comments to try and make things a bit clearer.

src/recovery.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
src/minmax.rs Outdated Show resolved Hide resolved
@lohith-bellad lohith-bellad force-pushed the windowed_minmax_filter branch 2 times, most recently from 9cceb4a to 627e2aa Compare March 13, 2020 16:23
@ghedo
recovery: windowed min and max filter implementation
df9b6cd 
Port google opensourced min-max filter code based on
Kathleen Nichols' algorithm for tracking the minimum
(or maximum) value of a data stream over some fixed
time interval.

Closes #305.
@ghedo ghedo merged commit 14bac3b into master Mar 16, 2020
@ghedo ghedo deleted the windowed_minmax_filter branch March 16, 2020 13:05
@ghedo
Copy link
Member

ghedo commented Mar 16, 2020 •
edited
Loading

For whatever reason rustc nightly seems to be crashing which causes the Travis CI builds to fail. Though that's probably unrelated to these changes (the crash happens when building dockopt) so I merged this anyway.

@ghedo
Copy link
Member

ghedo commented Mar 16, 2020 •
edited
Loading

FTR, I created rust-lang/rust#70041 to track the compiler crash.

@VMatrix1900 VMatrix1900 mentioned this pull request May 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LPardue LPardue LPardue left review comments

@ghedo ghedo ghedo approved these changes

@junhochoi junhochoi junhochoi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lohith-bellad @junhochoi @ghedo @LPardue