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

Standard deviation for aggregations #143

Merged
merged 27 commits into from
Aug 12, 2019
Merged

Standard deviation for aggregations #143

merged 27 commits into from
Aug 12, 2019

Conversation

ashtul
Copy link
Contributor

@ashtul ashtul commented Jun 19, 2019 •
edited
Loading

This PR is a copy of #125 which was opened on June 6th.

@ashtul
Copy link
Contributor Author

ashtul commented Jun 20, 2019

Part of enhancement requested on issue #84

@gkorland gkorland requested a review from danni-m June 23, 2019 06:59
@ashtul
Copy link
Contributor Author

ashtul commented Jul 7, 2019

@danni-m
Resolved conflict.

otnielvh
otnielvh reviewed Jul 8, 2019
View reviewed changes
src/compaction.c Outdated Show resolved Hide resolved
otnielvh
otnielvh reviewed Jul 8, 2019
View reviewed changes
src/compaction.c Outdated Show resolved Hide resolved
@ashtul
Copy link
Contributor Author

ashtul commented Jul 8, 2019

@danni-m,
@otnielvh have reviewed the code and asked for changes which were implemented.

@ashtul
Copy link
Contributor Author

ashtul commented Jul 17, 2019

@danni-m @gkorland
Can this branch be merged? It does not break API.

@gkorland gkorland requested review from otnielvh and removed request for itamarhaber July 22, 2019 11:06
danni-m
danni-m previously requested changes Jul 23, 2019
View reviewed changes
src/compaction.h Outdated Show resolved Hide resolved
src/consts.h Outdated Show resolved Hide resolved
@ashtul ashtul requested a review from danni-m July 23, 2019 08:29
@gkorland gkorland added this to Pending Review in 1.2 Jul 23, 2019
@ashtul ashtul added the enhancement New feature or request label Jul 23, 2019
@ashtul ashtul requested review from danni-m and removed request for otnielvh and danni-m July 24, 2019 10:40
otnielvh
otnielvh suggested changes Jul 30, 2019
View reviewed changes
Copy link

@otnielvh otnielvh left a comment

Choose a reason for hiding this comment

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

I went over the math and the math looks good. I recommend someone who is fluent in the TS module go over it to verify the best practices of TS module are met.

docs/commands.md Show resolved Hide resolved
docs/index.md Outdated Show resolved Hide resolved
src/compaction.c Show resolved Hide resolved
src/compaction.c Show resolved Hide resolved
src/compaction.c Outdated Show resolved Hide resolved
src/compaction.c Outdated Show resolved Hide resolved
src/compaction.c Outdated Show resolved Hide resolved
src/tests/test_module.py Show resolved Hide resolved
src/tests/test_module.py Outdated Show resolved Hide resolved
@ashtul ashtul requested a review from otnielvh August 3, 2019 20:31
otnielvh
otnielvh suggested changes Aug 4, 2019
View reviewed changes
src/compaction.c Outdated
@@ -122,7 +122,7 @@ double VarPopulationFinalize(void *contextPtr) {
if(count <= 1) {
return 0;
}
Copy link

Choose a reason for hiding this comment

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

This test should be handled by variance function (before dividing by count)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch!
Done.

src/compaction.c Outdated Show resolved Hide resolved
@otnielvh
Copy link

otnielvh commented Aug 4, 2019

Looks good, just one minor comment

@gkorland gkorland requested a review from otnielvh August 4, 2019 12:49
@ashtul
Copy link
Contributor Author

ashtul commented Aug 11, 2019

@otnielvh, as requested, made a random test with seed and epsilon.

otnielvh
otnielvh approved these changes Aug 12, 2019
View reviewed changes
src/tests/test_module.py
assert r.execute_command('TS.CREATERULE', raw_key, std_key, "AGGREGATION", 'std.s', random_numbers)
assert r.execute_command('TS.CREATERULE', raw_key, var_key, "AGGREGATION", 'var.s', random_numbers)

for i in range(random_numbers):
Copy link

Choose a reason for hiding this comment

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

len(items) would be clearer

@ashtul ashtul merged commit b835df9 into master Aug 12, 2019
@ashtul ashtul deleted the stdev branch August 12, 2019 09:49
@gkorland gkorland moved this from Pending Review to Done in 1.2 Aug 12, 2019
This was referenced Aug 12, 2019
Closed
Closed
Closed
@ashtul ashtul mentioned this pull request Aug 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@otnielvh otnielvh otnielvh approved these changes

@danni-m danni-m Awaiting requested review from danni-m danni-m is a code owner

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
No open projects
1.2
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ashtul @otnielvh @danni-m